adding MSI/MSIX for PCI on POWER

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 <[email protected]>

---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |   10 ++++++++++
 hw/msix.h |    3 +++
 4 files changed, 28 insertions(+), 0 deletions(-)

hw/msi.c +3 -0

index 5d6ceb6..124878a 100644
@@ -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)
+{

hw/msi.c +1 -0

+

hw/msi.h +1 -0

index 3040bb0..0acf434 100644
@@ -34,6 +34,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)
 {

hw/msix.c +10 -0

index 3835eaa..c57c299 100644
@@ -414,3 +414,13 @@ void msix_unuse_all_vectors(PCIDevice *dev)
         return;
     msix_free_irq_entries(dev);
 }
+
+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;
+}
+

hw/msix.h +3 -0

index 5aba22b..e6bb696 100644
@@ -29,4 +29,7 @@ void msix_notify(PCIDevice *dev, unsigned vector);
 
 void msix_reset(PCIDevice *dev);
 
+void msix_set_address_data(PCIDevice *dev, int vector,
+                           uint64_t address, uint32_t data);
+
 #endif

Quoted 37 lines · Alexey Kardashevskiy

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 <[email protected]>

---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |   10 ++++++++++
 hw/msix.h |    3 +++
 4 files changed, 28 insertions(+), 0 deletions(-)

hw/msi.c +9 -0

index 5d6ceb6..124878a 100644
@@ -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 {

hw/msi.c unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) · line 373

Quoted 4 lines

hw/msi.c +4 -0

+        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,

Alex

Quoted 43 lines

hw/msi.c +1 -0

+

hw/msi.h +1 -0

index 3040bb0..0acf434 100644
@@ -34,6 +34,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)
 {

hw/msix.c +10 -0

index 3835eaa..c57c299 100644
@@ -414,3 +414,13 @@ void msix_unuse_all_vectors(PCIDevice *dev)
         return;
     msix_free_irq_entries(dev);
 }
+
+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;
+}
+

hw/msix.h +3 -0

index 5aba22b..e6bb696 100644
@@ -29,4 +29,7 @@ void msix_notify(PCIDevice *dev, unsigned vector);
 
 void msix_reset(PCIDevice *dev);
 
+void msix_set_address_data(PCIDevice *dev, int vector,
+                           uint64_t address, uint32_t data);
+
 #endif

Quoted 38 lines · Alex Williamson

Quoted 37 lines · Alexey Kardashevskiy

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 <[email protected]>

---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |   10 ++++++++++
 hw/msix.h |    3 +++
 4 files changed, 28 insertions(+), 0 deletions(-)

hw/msi.c +9 -0

index 5d6ceb6..124878a 100644
@@ -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 {

Quoted 8 lines

hw/msi.c unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) · line 373

Quoted 4 lines

hw/msi.c +4 -0

+        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 :) I am totally unfamiliar with kvm_irqchip_add_msi_route to see the bigger picture, sorry.

Quoted 49 lines · Alexey Kardashevskiy

Quoted 38 lines · Alex Williamson

Quoted 37 lines · Alexey Kardashevskiy

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 <[email protected]>

---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |   10 ++++++++++
 hw/msix.h |    3 +++
 4 files changed, 28 insertions(+), 0 deletions(-)

hw/msi.c +9 -0

index 5d6ceb6..124878a 100644
@@ -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 {

Quoted 8 lines

hw/msi.c unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) · line 373

Quoted 4 lines

hw/msi.c +4 -0

+        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,

Quoted 3 lines

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 :) I am totally unfamiliar with kvm_irqchip_add_msi_route to see the bigger picture, sorry.

MSIVectorUseNotifier passes a MSIMessage back to the device when a vector is unmasked. We can then add a route in KVM for that message with kvm_irqchip_add_msi_route. Finally, kvm_irqchip_add_irqfd allows us to connect that MSI route to an eventfd, such as from virtio or vfio. Then MSI eventfds can bypass qemu and be injected directly into KVM and on into the guest. So we seem to already have some standardization on passing address/data via an MSIMessage.

You need a "set" interface, I need a "get" interface. msix already has a static msix_get_message(). So I'd suggest that an exported get/set_message for each seems like the right way to go. Thanks,

Quoted 62 lines · Alex Williamson

Quoted 49 lines · Alexey Kardashevskiy

Quoted 38 lines · Alex Williamson

Quoted 37 lines · Alexey Kardashevskiy

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 <[email protected]>

---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |   10 ++++++++++
 hw/msix.h |    3 +++
 4 files changed, 28 insertions(+), 0 deletions(-)

hw/msi.c +9 -0

index 5d6ceb6..124878a 100644
@@ -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 {

Quoted 8 lines

hw/msi.c unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) · line 373

Quoted 4 lines

hw/msi.c +4 -0

+        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,

Quoted 3 lines

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 :) I am totally unfamiliar with kvm_irqchip_add_msi_route to see the bigger picture, sorry.

MSIVectorUseNotifier passes a MSIMessage back to the device when a vector is unmasked. We can then add a route in KVM for that message with kvm_irqchip_add_msi_route. Finally, kvm_irqchip_add_irqfd allows us to connect that MSI route to an eventfd, such as from virtio or vfio. Then MSI eventfds can bypass qemu and be injected directly into KVM and on into the guest. So we seem to already have some standardization on passing address/data via an MSIMessage.

Quoted 3 lines

You need a "set" interface, I need a "get" interface. msix already has a static msix_get_message(). So I'd suggest that an exported get/set_message for each seems like the right way to go. Thanks,

Ok. Slowly :) What QEMU tree are you talking about? git, branch? There is neither MSIVectorUseNotifier nor MSIMessage in your or mine trees.

Quoted 38 lines · Alex Williamson

Quoted 37 lines · Alexey Kardashevskiy

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 <[email protected]>

---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |   10 ++++++++++
 hw/msix.h |    3 +++
 4 files changed, 28 insertions(+), 0 deletions(-)

hw/msi.c +9 -0

index 5d6ceb6..124878a 100644
@@ -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 {

Quoted 8 lines

hw/msi.c unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) · line 373

Quoted 4 lines

hw/msi.c +4 -0

+        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 :)

I am totally unfamiliar with kvm_irqchip_add_msi_route to see the bigger picture, sorry.

Quoted 49 lines · Alexey Kardashevskiy

Quoted 38 lines · Alex Williamson

Quoted 37 lines · Alexey Kardashevskiy

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 <[email protected]>

---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |   10 ++++++++++
 hw/msix.h |    3 +++
 4 files changed, 28 insertions(+), 0 deletions(-)

hw/msi.c +9 -0

index 5d6ceb6..124878a 100644
@@ -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 {

Quoted 8 lines

hw/msi.c unsigned int msi_nr_vectors_allocated(const PCIDevice *dev) · line 373

Quoted 4 lines

hw/msi.c +4 -0

+        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,

Quoted 2 lines

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.

Quoted 1 line

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

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 <[email protected]>

---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    8 ++++++++
 hw/msix.h |    3 +++
 4 files changed, 26 insertions(+)

hw/msi.c +14 -0

index 5233204..c7b3e6a 100644
@@ -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);
+}
+

hw/msi.h +1 -0

index 75747ab..353386e 100644
@@ -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)
 {

hw/msix.c +8 -0

index ded3c55..08e773d 100644
@@ -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;
+}

hw/msix.h +3 -0

index 50aee82..901f101 100644
@@ -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

Quoted 2 lines · Alexey Kardashevskiy

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

No - posted the old version accidentally?

Jan

Quoted 85 lines

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 <[email protected]>

---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    8 ++++++++
 hw/msix.h |    3 +++
 4 files changed, 26 insertions(+)

hw/msi.c +14 -0

index 5233204..c7b3e6a 100644
@@ -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);
+}
+

hw/msi.h +1 -0

index 75747ab..353386e 100644
@@ -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)
 {

hw/msix.c +8 -0

index ded3c55..08e773d 100644
@@ -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;
+}

hw/msix.h +3 -0

index 50aee82..901f101 100644
@@ -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

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644

hw/msi.c +17 -0

+
+    return msg;
+}
+
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    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), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +2 -0

index 75747ab..4b0f4f8 100644
@@ -31,6 +31,8 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+MSIMessage msi_get_message(PCIDevice *dev);
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +10 -1

index ded3c55..9e8d8bb 100644
@@ -35,7 +35,7 @@
 #define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2)
 #define MSIX_MAX_ENTRIES 32
 
-static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
+MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
 {
     uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
     MSIMessage msg;

@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +0 -0

index 50aee82..3374cf8 100644

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 119

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

See PCI spec and existing code.

Quoted 71 lines

hw/msi.c +17 -0

+
+    return msg;
+}
+
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    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), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +2 -0

index 75747ab..4b0f4f8 100644
@@ -31,6 +31,8 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+MSIMessage msi_get_message(PCIDevice *dev);
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +10 -1

index ded3c55..9e8d8bb 100644
@@ -35,7 +35,7 @@
 #define MSIX_PAGE_PENDING (MSIX_PAGE_SIZE / 2)
 #define MSIX_MAX_ENTRIES 32
 
-static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
+MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
 {
     uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
     MSIMessage msg;

@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +0 -0

index 50aee82..3374cf8 100644
@@ -4,6 +4,9 @@
 #include "qemu-common.h"
 #include "pci.h"
 

hw/msix.h line 12

Quoted 7 lines

hw/msix.h +3 -0

+MSIMessage msix_get_message(PCIDevice *dev, unsigned vector);
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

General remark: You will make the life of the maintainers easier by formatting your patch in a way that a clean merge via git works without hand-editing. E.g. this patch was no scissor line (---8<--- etc.) between introductory text and the patch description. And the subject is not "[PATCH] ...".

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 119

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

Quoted 67 lines · Alexey Kardashevskiy

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 119

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 5 lines

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

I'm not sure what your use case for reading the message is. For KVM device assignment it is preparing an alternative message delivery path for MSI vectors. And for this we will need vector notifier support for MSI as well. You can check the MSI-X code for corresponding use cases of msix_get_message.

And when we already have msi_get_message, another logical use case is msi_notify. See msix.c again.

Quoted 74 lines · Jan Kiszka

Quoted 67 lines · Alexey Kardashevskiy

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 119

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 5 lines

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

Quoted 8 lines

I'm not sure what your use case for reading the message is. For KVM device assignment it is preparing an alternative message delivery path for MSI vectors. And for this we will need vector notifier support for MSI as well. You can check the MSI-X code for corresponding use cases of msix_get_message.

And when we already have msi_get_message, another logical use case is msi_notify. See msix.c again.

Aaaa.

I have no case for reading the message. All I need is writing. And I want it public as I want to use it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" ("get" returns what "set" wrote). You want a different thing which I can do but it is not msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify().

Still can do what you suggested, it just does not seem right.

Quoted 86 lines · Alexey Kardashevskiy

Quoted 74 lines · Jan Kiszka

Quoted 67 lines · Alexey Kardashevskiy

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 119

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 5 lines

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

Quoted 8 lines

I'm not sure what your use case for reading the message is. For KVM device assignment it is preparing an alternative message delivery path for MSI vectors. And for this we will need vector notifier support for MSI as well. You can check the MSI-X code for corresponding use cases of msix_get_message.

And when we already have msi_get_message, another logical use case is msi_notify. See msix.c again.

Aaaa.

Quoted 7 lines

I have no case for reading the message. All I need is writing. And I want it public as I want to use it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" ("get" returns what "set" wrote). You want a different thing which I can do but it is not msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify().

Still can do what you suggested, it just does not seem right.

It is right - when looking at it from a different angle. ;)

I don't mind if you add msi_get_message now or leave this to me. Likely the latter is better as you have no use case for msi_get_message (and also msix_get_message!) outside of their modules, thus we should not export those functions anyway.

Jan

Added (msi|msix)_set_message() functions.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +13 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    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), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +0 -0

index 50aee82..26a437e 100644

Signature · 3 lines

1.7.10

ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly 

Quoted 82 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() functions.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +13 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    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), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +0 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 

hw/msix.h line 11

Quoted 6 lines

hw/msix.h +2 -0

+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

Interface looks good as fas as I can tell (can't asses the POWER need for clearing the mask bit on msix_set_message).

Quoted 4 lines

Signature · 3 lines

1.7.10

ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly 

Check man git-am.

Added (msi|msix)_set_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +13 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    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), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +2 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

Ping?

Quoted 83 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +13 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    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), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +0 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 

hw/msix.h line 11

Quoted 5 lines

hw/msix.h +2 -0

+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

Quoted 87 lines · Alexey Kardashevskiy

Ping?

Quoted 83 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---

hw/msi.c  |   13 +++++++++++++
hw/msi.h  |    1 +
hw/msix.c |    9 +++++++++
hw/msix.h |    2 ++
4 files changed, 25 insertions(+)

hw/msi.c +13 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
    return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    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), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
    return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
    return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
 * and fill MSI-X capability in the config space.

hw/msix.h +0 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 

Quoted 7 lines

hw/msix.h line 11

Quoted 5 lines

hw/msix.h +2 -0

+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
              MemoryRegion *bar,
              unsigned bar_nr, unsigned bar_size);

Acked-by Jan Kiszka <[email protected]>

Quoted 96 lines · Jan Kiszka

Quoted 94 lines · Alexey Kardashevskiy

Ping?

Quoted 88 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---

hw/msi.c  |   13 +++++++++++++
hw/msi.h  |    1 +
hw/msix.c |    9 +++++++++
hw/msix.h |    2 ++
4 files changed, 25 insertions(+)

hw/msi.c +13 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
    return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    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), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
    return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
             unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
    return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
 * and fill MSI-X capability in the config space.

hw/msix.h +2 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
              MemoryRegion *bar,
              unsigned bar_nr, unsigned bar_size);

Quoted 1 line

Acked-by Jan Kiszka <[email protected]>

So I take it that you expect this patch to go through my tree, not yours?

Quoted 99 lines · Alexander Graf

Quoted 96 lines · Jan Kiszka

Quoted 94 lines · Alexey Kardashevskiy

Ping?

Quoted 88 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---

hw/msi.c  |   13 +++++++++++++
hw/msi.h  |    1 +
hw/msix.c |    9 +++++++++
hw/msix.h |    2 ++
4 files changed, 25 insertions(+)

hw/msi.c +13 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
   return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    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), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+
 bool msi_enabled(const PCIDevice *dev)
 {
   return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
            unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
   return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
 * and fill MSI-X capability in the config space.

hw/msix.h +2 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
             MemoryRegion *bar,
             unsigned bar_nr, unsigned bar_size);

Quoted 3 lines

Quoted 1 line

Acked-by Jan Kiszka <[email protected]>

So I take it that you expect this patch to go through my tree, not yours?

I suppose you wanted to address Michael, right? He has to decide.

Added (msi|msix)_set_message() functions.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +3 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));

hw/msi.c +0 -0

 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +2 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

Signature · 3 lines

1.7.10

ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)

Quoted 104 lines · Jan Kiszka

Quoted 86 lines · Alexey Kardashevskiy

Quoted 74 lines · Jan Kiszka

Quoted 67 lines · Alexey Kardashevskiy

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 5 lines

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

Quoted 8 lines

I'm not sure what your use case for reading the message is. For KVM device assignment it is preparing an alternative message delivery path for MSI vectors. And for this we will need vector notifier support for MSI as well. You can check the MSI-X code for corresponding use cases of msix_get_message.

And when we already have msi_get_message, another logical use case is msi_notify. See msix.c again.

Aaaa.

Quoted 7 lines

I have no case for reading the message. All I need is writing. And I want it public as I want to use it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" ("get" returns what "set" wrote). You want a different thing which I can do but it is not msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify().

Still can do what you suggested, it just does not seem right.

It is right - when looking at it from a different angle. ;)

I don't mind if you add msi_get_message now or leave this to me. Likely the latter is better as you have no use case for msi_get_message (and also msix_get_message!) outside of their modules, thus we should not export those functions anyway.

Jan

Signature · 1 line

Alexey

Quoted 6 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() functions.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

Quoted 5 lines

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

So guests do enable MSI through config space, but do not fill in vectors?

Very strange. Are you sure it's not
just a guest bug? How does it work for other PCI devices?

Can't we just fix guest drivers to program the vectors properly?

Also pls address the comment below.

Thanks!

Quoted 18 lines

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +3 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 120

Quoted 10 lines

hw/msi.c +10 -0

+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+
+    if (msi64bit) {
+        pci_set_quad(dev->config + msi_address_lo_off(dev), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+

Please add documentation. Something like

/*
* Special API for POWER to configure the vectors through
* a side channel. Should never be used by devices. */

Quoted 160 lines

hw/msi.c +0 -0

 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +2 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

Signature · 3 lines

1.7.10

ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)

Quoted 101 lines · Jan Kiszka

Quoted 86 lines · Alexey Kardashevskiy

Quoted 74 lines · Jan Kiszka

Quoted 67 lines · Alexey Kardashevskiy

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 5 lines

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

Quoted 8 lines

I'm not sure what your use case for reading the message is. For KVM device assignment it is preparing an alternative message delivery path for MSI vectors. And for this we will need vector notifier support for MSI as well. You can check the MSI-X code for corresponding use cases of msix_get_message.

And when we already have msi_get_message, another logical use case is msi_notify. See msix.c again.

Aaaa.

Quoted 7 lines

I have no case for reading the message. All I need is writing. And I want it public as I want to use it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" ("get" returns what "set" wrote). You want a different thing which I can do but it is not msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify().

Still can do what you suggested, it just does not seem right.

It is right - when looking at it from a different angle. ;)

I don't mind if you add msi_get_message now or leave this to me. Likely the latter is better as you have no use case for msi_get_message (and also msix_get_message!) outside of their modules, thus we should not export those functions anyway.

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() functions.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Quoted 4 lines

Quoted 1 line

Signed-off-by Alexey Kardashevskiy <[email protected]>

So guests do enable MSI through config space, but do not fill in vectors?

Yes. msix_capability_init() calls arch_setup_msi_irqs() which does everything it needs to do (i.e. calls hypervisor) before msix_capability_init() writes PCI_MSIX_FLAGS_ENABLE to the PCI_MSIX_FLAGS register.

These vectors are the PCI bus addresses, the way they are set is specific for a PCI host controller, I do not see why the current scheme is a bug.

Quoted 2 lines

Very strange. Are you sure it's not
just a guest bug? How does it work for other PCI devices?

Did not get the question. It works the same for every PCI device under POWER guest.

Quoted 3 lines

Can't we just fix guest drivers to program the vectors properly?

Also pls address the comment below.

Comment below.

Quoted 33 lines

Thanks!

Quoted 18 lines

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +3 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 120

Quoted 10 lines

hw/msi.c +10 -0

+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+
+    if (msi64bit) {
+        pci_set_quad(dev->config + msi_address_lo_off(dev), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+

Please add documentation. Something like

Quoted 4 lines

/*
* Special API for POWER to configure the vectors through
* a side channel. Should never be used by devices. */

It is useful for any para-virtualized environment I believe, is not it?
For s390 as well. Of course, if it supports PCI, for example, what I am not sure it does though :)

Quoted 160 lines

Quoted 160 lines

hw/msi.c +0 -0

 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +2 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

Signature · 3 lines

1.7.10

ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)

Quoted 101 lines · Jan Kiszka

Quoted 86 lines · Alexey Kardashevskiy

Quoted 74 lines · Jan Kiszka

Quoted 67 lines · Alexey Kardashevskiy

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 5 lines

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

Quoted 8 lines

I'm not sure what your use case for reading the message is. For KVM device assignment it is preparing an alternative message delivery path for MSI vectors. And for this we will need vector notifier support for MSI as well. You can check the MSI-X code for corresponding use cases of msix_get_message.

And when we already have msi_get_message, another logical use case is msi_notify. See msix.c again.

Aaaa.

Quoted 7 lines

I have no case for reading the message. All I need is writing. And I want it public as I want to use it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" ("get" returns what "set" wrote). You want a different thing which I can do but it is not msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify().

Still can do what you suggested, it just does not seem right.

It is right - when looking at it from a different angle. ;)

I don't mind if you add msi_get_message now or leave this to me. Likely the latter is better as you have no use case for msi_get_message (and also msix_get_message!) outside of their modules, thus we should not export those functions anyway.

Signature · 1 line

Alexey

Quoted 17 lines · Alexey Kardashevskiy

Quoted 15 lines · Michael S. Tsirkin

Quoted 11 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() functions.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

So guests do enable MSI through config space, but do not fill in vectors?

Quoted 3 lines

Yes. msix_capability_init() calls arch_setup_msi_irqs() which does everything it needs to do (i.e. calls hypervisor) before msix_capability_init() writes PCI_MSIX_FLAGS_ENABLE to the PCI_MSIX_FLAGS register.

These vectors are the PCI bus addresses, the way they are set is specific for a PCI host controller, I do not see why the current scheme is a bug.

I won't work with any real PCI device, will it? Real pci devices expect vectors to be written into their memory.

Quoted 4 lines

Quoted 2 lines

Very strange. Are you sure it's not
just a guest bug? How does it work for other PCI devices?

Did not get the question. It works the same for every PCI device under POWER guest.

I mean for real PCI devices.

Quoted 45 lines

Quoted 3 lines

Can't we just fix guest drivers to program the vectors properly?

Also pls address the comment below.

Comment below.

Quoted 37 lines

Thanks!

Quoted 18 lines

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +3 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 120

Quoted 10 lines

hw/msi.c +10 -0

+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+
+    if (msi64bit) {
+        pci_set_quad(dev->config + msi_address_lo_off(dev), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+

Please add documentation. Something like

/*
* Special API for POWER to configure the vectors through
* a side channel. Should never be used by devices. */

Quoted 2 lines

It is useful for any para-virtualized environment I believe, is not it?
For s390 as well. Of course, if it supports PCI, for example, what I am not sure it does though :)

I expect the normal guest to program the address into MSI register using config accesses, same way that it enables MSI/MSIX.
Why POWER does it differently I did not yet figure out but I hope this weirdness is not so widespread.

Quoted 160 lines

Quoted 160 lines

Quoted 160 lines

hw/msi.c +0 -0

 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +2 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

Signature · 3 lines

1.7.10

ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)

Quoted 101 lines · Jan Kiszka

Quoted 86 lines · Alexey Kardashevskiy

Quoted 74 lines · Jan Kiszka

Quoted 67 lines · Alexey Kardashevskiy

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 5 lines

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

Quoted 8 lines

I'm not sure what your use case for reading the message is. For KVM device assignment it is preparing an alternative message delivery path for MSI vectors. And for this we will need vector notifier support for MSI as well. You can check the MSI-X code for corresponding use cases of msix_get_message.

And when we already have msi_get_message, another logical use case is msi_notify. See msix.c again.

Aaaa.

Quoted 7 lines

I have no case for reading the message. All I need is writing. And I want it public as I want to use it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" ("get" returns what "set" wrote). You want a different thing which I can do but it is not msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify().

Still can do what you suggested, it just does not seem right.

It is right - when looking at it from a different angle. ;)

I don't mind if you add msi_get_message now or leave this to me. Likely the latter is better as you have no use case for msi_get_message (and also msix_get_message!) outside of their modules, thus we should not export those functions anyway.

Quoted 22 lines · Michael S. Tsirkin

Quoted 20 lines · Alexey Kardashevskiy

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() functions.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

Quoted 8 lines

Quoted 5 lines

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

So guests do enable MSI through config space, but do not fill in vectors?

Yes. msix_capability_init() calls arch_setup_msi_irqs() which does everything it needs to do (i.e. calls hypervisor) before msix_capability_init() writes PCI_MSIX_FLAGS_ENABLE to the PCI_MSIX_FLAGS register.

These vectors are the PCI bus addresses, the way they are set is specific for a PCI host controller, I do not see why the current scheme is a bug.

Quoted 2 lines

I won't work with any real PCI device, will it? Real pci devices expect vectors to be written into their memory.

Yes. And the hypervisor does this. On POWER (at least book3s - server powerpc, the whole config space kitchen is hidden behind RTAS (kind of bios). For the guest, this RTAS is implemented in hypervisor, for the host - in the system firmware. So powerpc linux does not have to have PHB drivers. Kinda cool.

Usual powerpc server is running without the host linux at all, it is running a hypervisor called pHyp. And every guest knows that it is a guest, there is no full machine emulation, it is para-virtualization. In power-kvm, we replace that pHyp with the host linux and now QEMU plays a hypervisor role. Some day We will move the hypervisor to the host kernel completely (?) but now it is in QEMU.

Quoted 55 lines

Quoted 4 lines

Quoted 2 lines

Very strange. Are you sure it's not
just a guest bug? How does it work for other PCI devices?

Did not get the question. It works the same for every PCI device under POWER guest.

I mean for real PCI devices.

Quoted 47 lines

Quoted 3 lines

Can't we just fix guest drivers to program the vectors properly?

Also pls address the comment below.

Comment below.

Quoted 33 lines

Thanks!

Quoted 18 lines

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +3 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 120

Quoted 10 lines

hw/msi.c +10 -0

+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+
+    if (msi64bit) {
+        pci_set_quad(dev->config + msi_address_lo_off(dev), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+

Please add documentation. Something like

Quoted 4 lines

/*
* Special API for POWER to configure the vectors through
* a side channel. Should never be used by devices. */

It is useful for any para-virtualized environment I believe, is not it?
For s390 as well. Of course, if it supports PCI, for example, what I am not sure it does though :)

Quoted 4 lines

I expect the normal guest to program the address into MSI register using config accesses, same way that it enables MSI/MSIX.
Why POWER does it differently I did not yet figure out but I hope this weirdness is not so widespread.

In para-virt I would expect the guest not to touch config space at all. At least it should use one interface rather than two but this is how it is.

Quoted 160 lines

Quoted 160 lines

Quoted 160 lines

Quoted 160 lines

hw/msi.c +0 -0

 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +2 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

Signature · 3 lines

1.7.10

ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)

Quoted 101 lines · Jan Kiszka

Quoted 86 lines · Alexey Kardashevskiy

Quoted 74 lines · Jan Kiszka

Quoted 67 lines · Alexey Kardashevskiy

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 5 lines

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

Quoted 8 lines

I'm not sure what your use case for reading the message is. For KVM device assignment it is preparing an alternative message delivery path for MSI vectors. And for this we will need vector notifier support for MSI as well. You can check the MSI-X code for corresponding use cases of msix_get_message.

And when we already have msi_get_message, another logical use case is msi_notify. See msix.c again.

Aaaa.

Quoted 7 lines

I have no case for reading the message. All I need is writing. And I want it public as I want to use it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" ("get" returns what "set" wrote). You want a different thing which I can do but it is not msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify().

Still can do what you suggested, it just does not seem right.

It is right - when looking at it from a different angle. ;)

I don't mind if you add msi_get_message now or leave this to me. Likely the latter is better as you have no use case for msi_get_message (and also msix_get_message!) outside of their modules, thus we should not export those functions anyway.

Signature · 1 line

Alexey

Quoted 23 lines · Alexey Kardashevskiy

Quoted 22 lines · Michael S. Tsirkin

Quoted 20 lines · Alexey Kardashevskiy

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() functions.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

Quoted 8 lines

Quoted 5 lines

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

So guests do enable MSI through config space, but do not fill in vectors?

Yes. msix_capability_init() calls arch_setup_msi_irqs() which does everything it needs to do (i.e. calls hypervisor) before msix_capability_init() writes PCI_MSIX_FLAGS_ENABLE to the PCI_MSIX_FLAGS register.

These vectors are the PCI bus addresses, the way they are set is specific for a PCI host controller, I do not see why the current scheme is a bug.

Quoted 7 lines

Quoted 2 lines

I won't work with any real PCI device, will it? Real pci devices expect vectors to be written into their memory.

Yes. And the hypervisor does this. On POWER (at least book3s - server powerpc, the whole config space kitchen is hidden behind RTAS (kind of bios). For the guest, this RTAS is implemented in hypervisor, for the host - in the system firmware. So powerpc linux does not have to have PHB drivers. Kinda cool.

Usual powerpc server is running without the host linux at all, it is running a hypervisor called pHyp. And every guest knows that it is a guest, there is no full machine emulation, it is para-virtualization. In power-kvm, we replace that pHyp with the host linux and now QEMU plays a hypervisor role. Some day We will move the hypervisor to the host kernel completely (?) but now it is in QEMU.

OKay. So it is a POWER-specific weirdness as I suspected. Sure, if this is what real hardware does we pretty much have to emulate this.

Quoted 61 lines

Quoted 59 lines

Quoted 4 lines

Quoted 2 lines

Very strange. Are you sure it's not
just a guest bug? How does it work for other PCI devices?

Did not get the question. It works the same for every PCI device under POWER guest.

I mean for real PCI devices.

Quoted 39 lines

Quoted 3 lines

Can't we just fix guest drivers to program the vectors properly?

Also pls address the comment below.

Comment below.

Quoted 33 lines

Thanks!

Quoted 18 lines

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +3 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 120

Quoted 10 lines

hw/msi.c +10 -0

+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+
+    if (msi64bit) {
+        pci_set_quad(dev->config + msi_address_lo_off(dev), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+

Please add documentation. Something like

Quoted 8 lines

Quoted 4 lines

/*
* Special API for POWER to configure the vectors through
* a side channel. Should never be used by devices. */

It is useful for any para-virtualized environment I believe, is not it?
For s390 as well. Of course, if it supports PCI, for example, what I am not sure it does though :)

I expect the normal guest to program the address into MSI register using config accesses, same way that it enables MSI/MSIX.
Why POWER does it differently I did not yet figure out but I hope this weirdness is not so widespread.

Quoted 1 line

In para-virt I would expect the guest not to touch config space at all. At least it should use one interface rather than two but this is how it is.

It's not new that firmware developers consistently make inconsistent design decisions :)

Quoted 160 lines

Quoted 160 lines

Quoted 160 lines

Quoted 160 lines

Quoted 160 lines

hw/msi.c +0 -0

 bool msi_enabled(const PCIDevice *dev)
 {
     return msi_present(dev) &&

hw/msi.h +1 -0

index 75747ab..6ec1f99 100644
@@ -31,6 +31,7 @@ struct MSIMessage {
 
 extern bool msi_supported;
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg);
 bool msi_enabled(const PCIDevice *dev);
 int msi_init(struct PCIDevice *dev, uint8_t offset,
              unsigned int nr_vectors, bool msi64bit, bool msi_per_vector_mask);

hw/msix.c +9 -0

index ded3c55..5f7d6d3 100644
@@ -45,6 +45,15 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
+
+    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, msg.address);
+    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, msg.data);
+    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
+}
+
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
  * and fill MSI-X capability in the config space.

hw/msix.h +2 -0

index 50aee82..26a437e 100644
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
+
 int msix_init(PCIDevice *pdev, unsigned short nentries,
               MemoryRegion *bar,
               unsigned bar_nr, unsigned bar_size);

Signature · 3 lines

1.7.10

ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)

Quoted 101 lines · Jan Kiszka

Quoted 86 lines · Alexey Kardashevskiy

Quoted 74 lines · Jan Kiszka

Quoted 67 lines · Alexey Kardashevskiy

Quoted 36 lines · Jan Kiszka

Quoted 35 lines · Alexey Kardashevskiy

agrhhh. sha1 of the patch changed after rebasing :)

Added (msi|msix)_(set|get)_message() function for whoever might want to use them.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

As only set* function are required by now, the "get" functions were added or made public for a symmetry.

Signed-off-by Alexey Kardashevskiy <[email protected]>

---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

hw/msi.c +0 -0

index 5233204..9ad84a4 100644
@@ -105,6 +105,35 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 

Quoted 3 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 108

Quoted 1 line

hw/msi.c +1 -0

+MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

Who/how/why is going to calculate the vector here?

Quoted 23 lines

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)

Quoted 12 lines

hw/msi.c +11 -0

+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+    MSIMessage msg;
+
+    if (msi64bit) {
+        msg.address = pci_get_quad(dev->config + msi_address_lo_off(dev));
+    } else {
+        msg.address = pci_get_long(dev->config + msi_address_lo_off(dev));
+    }
+    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

And I have this here in addition:

    unsigned int nr_vectors = msi_nr_vectors(flags);
    ...

    if (nr_vectors > 1) {
        msg.data &= ~(nr_vectors - 1);
        msg.data |= vector;
    }

Quoted 5 lines

Quoted 1 line

See PCI spec and existing code.

What for? I really do not get it why someone might want to read something but not real value. What PCI code should I look?

Quoted 8 lines

I'm not sure what your use case for reading the message is. For KVM device assignment it is preparing an alternative message delivery path for MSI vectors. And for this we will need vector notifier support for MSI as well. You can check the MSI-X code for corresponding use cases of msix_get_message.

And when we already have msi_get_message, another logical use case is msi_notify. See msix.c again.

Aaaa.

Quoted 7 lines

I have no case for reading the message. All I need is writing. And I want it public as I want to use it from hw/spapr_pci.c. You suggested to add reading, I added "get" to be _symmetric_ to "set" ("get" returns what "set" wrote). You want a different thing which I can do but it is not msi_get_message(), it is something like msi_prepare_message(MSImessage msg) or msi_set_vector(uint16_t data) or simply internal kitchen of msi_notify().

Still can do what you suggested, it just does not seem right.

It is right - when looking at it from a different angle. ;)

I don't mind if you add msi_get_message now or leave this to me. Likely the latter is better as you have no use case for msi_get_message (and also msix_get_message!) outside of their modules, thus we should not export those functions anyway.

One comment below.

Quoted 97 lines · Michael S. Tsirkin

Quoted 23 lines · Alexey Kardashevskiy

Quoted 22 lines · Michael S. Tsirkin

Quoted 20 lines · Alexey Kardashevskiy

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Alexey Kardashevskiy

Added (msi|msix)_set_message() functions.

Currently msi_notify()/msix_notify() write to these vectors to signal the guest about an interrupt so the correct values have to written there by the guest or QEMU.

Quoted 8 lines

Quoted 5 lines

For example, POWER guest never initializes MSI/MSIX vectors, instead it uses RTAS hypercalls. So in order to support MSIX for virtio-pci on POWER we have to initialize MSI/MSIX message from QEMU.

Signed-off-by Alexey Kardashevskiy <[email protected]>

So guests do enable MSI through config space, but do not fill in vectors?

Yes. msix_capability_init() calls arch_setup_msi_irqs() which does everything it needs to do (i.e. calls hypervisor) before msix_capability_init() writes PCI_MSIX_FLAGS_ENABLE to the PCI_MSIX_FLAGS register.

These vectors are the PCI bus addresses, the way they are set is specific for a PCI host controller, I do not see why the current scheme is a bug.

Quoted 7 lines

Quoted 2 lines

I won't work with any real PCI device, will it? Real pci devices expect vectors to be written into their memory.

Yes. And the hypervisor does this. On POWER (at least book3s - server powerpc, the whole config space kitchen is hidden behind RTAS (kind of bios). For the guest, this RTAS is implemented in hypervisor, for the host - in the system firmware. So powerpc linux does not have to have PHB drivers. Kinda cool.

Usual powerpc server is running without the host linux at all, it is running a hypervisor called pHyp. And every guest knows that it is a guest, there is no full machine emulation, it is para-virtualization. In power-kvm, we replace that pHyp with the host linux and now QEMU plays a hypervisor role. Some day We will move the hypervisor to the host kernel completely (?) but now it is in QEMU.

OKay. So it is a POWER-specific weirdness as I suspected. Sure, if this is what real hardware does we pretty much have to emulate this.

Quoted 61 lines

Quoted 59 lines

Quoted 4 lines

Quoted 2 lines

Very strange. Are you sure it's not
just a guest bug? How does it work for other PCI devices?

Did not get the question. It works the same for every PCI device under POWER guest.

I mean for real PCI devices.

Quoted 39 lines

Quoted 3 lines

Can't we just fix guest drivers to program the vectors properly?

Also pls address the comment below.

Comment below.

Quoted 33 lines

Thanks!

Quoted 18 lines

---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

hw/msi.c +3 -0

index 5233204..cc6102f 100644
@@ -105,6 +105,19 @@ static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit)
     return dev->msi_cap + (msi64bit ? PCI_MSI_PENDING_64 : PCI_MSI_PENDING_32);
 }
 
+void msi_set_message(PCIDevice *dev, MSIMessage msg)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));

hw/msi.c static inline uint8_t msi_pending_off(const PCIDevice* dev, bool msi64bit) · line 120

Quoted 10 lines

hw/msi.c +10 -0

+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+
+    if (msi64bit) {
+        pci_set_quad(dev->config + msi_address_lo_off(dev), msg.address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), msg.address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), msg.data);
+}
+

Please add documentation. Something like

Quoted 8 lines

Quoted 4 lines

/*
* Special API for POWER to configure the vectors through
* a side channel. Should never be used by devices. */

It is useful for any para-virtualized environment I believe, is not it?
For s390 as well. Of course, if it supports PCI, for example, what I am not sure it does though :)

I expect the normal guest to program the address into MSI register using config accesses, same way that it enables MSI/MSIX.
Why POWER does it differently I did not yet figure out but I hope this weirdness is not so widespread.