[Qemu-devel] [PATCH 0/3] adding MSI/MSIX for PCI on POWER

[Qemu-devel] [PATCH 0/3] adding MSI/MSIX for PCI on POWER

[Alexey Kardashevskiy]

The following patches add MSIX support for PCI on POWER.
The first aim is virtio-pci so it was tested. It will also support
VFIO when it becomes available in public.

Alexey Kardashevskiy (3):
  msi/msix: added functions to API to set up message address and data
  pseries: added allocator for a block of IRQs
  pseries pci: added MSI/MSIX support

 hw/msi.c       |   14 +++
 hw/msi.h       |    1 +
 hw/msix.c      |   10 ++
 hw/msix.h      |    3 +
 hw/spapr.c     |   26 +++++-
 hw/spapr.h     |    1 +
 hw/spapr_pci.c |  266 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/spapr_pci.h |   13 +++-
 trace-events   |    9 ++
 9 files changed, 331 insertions(+), 12 deletions(-)

-- 
1.7.7.3

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

[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 <aik@ozlabs.ru>
---
 hw/msi.c  |   14 ++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |   10 ++++++++++
 hw/msix.h |    3 +++
 4 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/hw/msi.c b/hw/msi.c
index 5d6ceb6..124878a 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
     uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
     return msi_nr_vectors(flags);
 }
+
+void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
+
+    if (msi64bit) {
+        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
+    } else {
+        pci_set_long(dev->config + msi_address_lo_off(dev), address);
+    }
+    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
+}
+
diff --git a/hw/msi.h b/hw/msi.h
index 3040bb0..0acf434 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -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)
 {
diff --git a/hw/msix.c b/hw/msix.c
index 3835eaa..c57c299 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -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;
+}
+
diff --git a/hw/msix.h b/hw/msix.h
index 5aba22b..e6bb696 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -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
-- 
1.7.7.3

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

[Alex Williamson]

On Thu, 2012-06-14 at 14:31 +1000, Alexey Kardashevskiy wrote:
> Normally QEMU expects the guest to initialize MSI/MSIX vectors.
> However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and
> does not write these vectors to device's config space or MSIX BAR.
> 
> On the other hand, msi_notify()/msix_notify() write to these vectors to
> signal the guest about an interrupt so we have to write correct vectors
> to the devices in order not to change every user of MSI/MSIX.
> 
> The first aim is to support MSIX for virtio-pci on POWER. There is
> another patch for POWER coming which introduces a special memory region
> where MSI/MSIX vectors point to.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/msi.c  |   14 ++++++++++++++
>  hw/msi.h  |    1 +
>  hw/msix.c |   10 ++++++++++
>  hw/msix.h |    3 +++
>  4 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/msi.c b/hw/msi.c
> index 5d6ceb6..124878a 100644
> --- a/hw/msi.c
> +++ b/hw/msi.c
> @@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>      return msi_nr_vectors(flags);
>  }
> +
> +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
> +{
> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> +
> +    if (msi64bit) {
> +        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
> +    } else {
> +        pci_set_long(dev->config + msi_address_lo_off(dev), address);
> +    }
> +    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
> +}

Why not make it msi_set_message() and pass MSIMessage?  I'd be great if
you tossed in a msi_get_message() as well, I think we need it to be able
to do a kvm_irqchip_add_msi_route() with MSI.  Thanks,

Alex

> +
> diff --git a/hw/msi.h b/hw/msi.h
> index 3040bb0..0acf434 100644
> --- a/hw/msi.h
> +++ b/hw/msi.h
> @@ -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)
>  {
> diff --git a/hw/msix.c b/hw/msix.c
> index 3835eaa..c57c299 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -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;
> +}
> +
> diff --git a/hw/msix.h b/hw/msix.h
> index 5aba22b..e6bb696 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -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

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

[Alexey Kardashevskiy]

On 14/06/12 14:56, Alex Williamson wrote:
> On Thu, 2012-06-14 at 14:31 +1000, Alexey Kardashevskiy wrote:
>> Normally QEMU expects the guest to initialize MSI/MSIX vectors.
>> However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and
>> does not write these vectors to device's config space or MSIX BAR.
>>
>> On the other hand, msi_notify()/msix_notify() write to these vectors to
>> signal the guest about an interrupt so we have to write correct vectors
>> to the devices in order not to change every user of MSI/MSIX.
>>
>> The first aim is to support MSIX for virtio-pci on POWER. There is
>> another patch for POWER coming which introduces a special memory region
>> where MSI/MSIX vectors point to.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  hw/msi.c  |   14 ++++++++++++++
>>  hw/msi.h  |    1 +
>>  hw/msix.c |   10 ++++++++++
>>  hw/msix.h |    3 +++
>>  4 files changed, 28 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/msi.c b/hw/msi.c
>> index 5d6ceb6..124878a 100644
>> --- a/hw/msi.c
>> +++ b/hw/msi.c
>> @@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
>>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>      return msi_nr_vectors(flags);
>>  }
>> +
>> +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
>> +{
>> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>> +
>> +    if (msi64bit) {
>> +        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
>> +    } else {
>> +        pci_set_long(dev->config + msi_address_lo_off(dev), address);
>> +    }
>> +    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
>> +}
> 
> Why not make it msi_set_message() and pass MSIMessage?  I'd be great if
> you tossed in a msi_get_message() as well, I think we need it to be able
> to do a kvm_irqchip_add_msi_route() with MSI.  Thanks,


I am missing the point. What is that MSIMessage?
It is just an address and data, making a struct from this is a bit too much :)
I am totally unfamiliar with kvm_irqchip_add_msi_route to see the bigger picture, sorry.


> Alex
> 
>> +
>> diff --git a/hw/msi.h b/hw/msi.h
>> index 3040bb0..0acf434 100644
>> --- a/hw/msi.h
>> +++ b/hw/msi.h
>> @@ -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)
>>  {
>> diff --git a/hw/msix.c b/hw/msix.c
>> index 3835eaa..c57c299 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -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;
>> +}
>> +
>> diff --git a/hw/msix.h b/hw/msix.h
>> index 5aba22b..e6bb696 100644
>> --- a/hw/msix.h
>> +++ b/hw/msix.h
>> @@ -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
> 
> 
> 


-- 
Alexey

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

[Alex Williamson]

On Thu, 2012-06-14 at 15:17 +1000, Alexey Kardashevskiy wrote:
> On 14/06/12 14:56, Alex Williamson wrote:
> > On Thu, 2012-06-14 at 14:31 +1000, Alexey Kardashevskiy wrote:
> >> Normally QEMU expects the guest to initialize MSI/MSIX vectors.
> >> However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and
> >> does not write these vectors to device's config space or MSIX BAR.
> >>
> >> On the other hand, msi_notify()/msix_notify() write to these vectors to
> >> signal the guest about an interrupt so we have to write correct vectors
> >> to the devices in order not to change every user of MSI/MSIX.
> >>
> >> The first aim is to support MSIX for virtio-pci on POWER. There is
> >> another patch for POWER coming which introduces a special memory region
> >> where MSI/MSIX vectors point to.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >>  hw/msi.c  |   14 ++++++++++++++
> >>  hw/msi.h  |    1 +
> >>  hw/msix.c |   10 ++++++++++
> >>  hw/msix.h |    3 +++
> >>  4 files changed, 28 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hw/msi.c b/hw/msi.c
> >> index 5d6ceb6..124878a 100644
> >> --- a/hw/msi.c
> >> +++ b/hw/msi.c
> >> @@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
> >>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> >>      return msi_nr_vectors(flags);
> >>  }
> >> +
> >> +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
> >> +{
> >> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> >> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> >> +
> >> +    if (msi64bit) {
> >> +        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
> >> +    } else {
> >> +        pci_set_long(dev->config + msi_address_lo_off(dev), address);
> >> +    }
> >> +    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
> >> +}
> > 
> > Why not make it msi_set_message() and pass MSIMessage?  I'd be great if
> > you tossed in a msi_get_message() as well, I think we need it to be able
> > to do a kvm_irqchip_add_msi_route() with MSI.  Thanks,
> 
> 
> I am missing the point. What is that MSIMessage?
> It is just an address and data, making a struct from this is a bit too much :)
> 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,

Alex

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

[Alexey Kardashevskiy]

On 14/06/12 15:38, Alex Williamson wrote:
> On Thu, 2012-06-14 at 15:17 +1000, Alexey Kardashevskiy wrote:
>> On 14/06/12 14:56, Alex Williamson wrote:
>>> On Thu, 2012-06-14 at 14:31 +1000, Alexey Kardashevskiy wrote:
>>>> Normally QEMU expects the guest to initialize MSI/MSIX vectors.
>>>> However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and
>>>> does not write these vectors to device's config space or MSIX BAR.
>>>>
>>>> On the other hand, msi_notify()/msix_notify() write to these vectors to
>>>> signal the guest about an interrupt so we have to write correct vectors
>>>> to the devices in order not to change every user of MSI/MSIX.
>>>>
>>>> The first aim is to support MSIX for virtio-pci on POWER. There is
>>>> another patch for POWER coming which introduces a special memory region
>>>> where MSI/MSIX vectors point to.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>>  hw/msi.c  |   14 ++++++++++++++
>>>>  hw/msi.h  |    1 +
>>>>  hw/msix.c |   10 ++++++++++
>>>>  hw/msix.h |    3 +++
>>>>  4 files changed, 28 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>> index 5d6ceb6..124878a 100644
>>>> --- a/hw/msi.c
>>>> +++ b/hw/msi.c
>>>> @@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
>>>>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>>>      return msi_nr_vectors(flags);
>>>>  }
>>>> +
>>>> +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
>>>> +{
>>>> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>>> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>>>> +
>>>> +    if (msi64bit) {
>>>> +        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
>>>> +    } else {
>>>> +        pci_set_long(dev->config + msi_address_lo_off(dev), address);
>>>> +    }
>>>> +    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
>>>> +}
>>>
>>> Why not make it msi_set_message() and pass MSIMessage?  I'd be great if
>>> you tossed in a msi_get_message() as well, I think we need it to be able
>>> to do a kvm_irqchip_add_msi_route() with MSI.  Thanks,
>>
>>
>> I am missing the point. What is that MSIMessage?
>> It is just an address and data, making a struct from this is a bit too much :)
>> 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,

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


-- 
Alexey

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

[Alex Williamson]

On Thu, 2012-06-14 at 15:44 +1000, Alexey Kardashevskiy wrote:
> On 14/06/12 15:38, Alex Williamson wrote:
> > On Thu, 2012-06-14 at 15:17 +1000, Alexey Kardashevskiy wrote:
> >> On 14/06/12 14:56, Alex Williamson wrote:
> >>> On Thu, 2012-06-14 at 14:31 +1000, Alexey Kardashevskiy wrote:
> >>>> Normally QEMU expects the guest to initialize MSI/MSIX vectors.
> >>>> However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and
> >>>> does not write these vectors to device's config space or MSIX BAR.
> >>>>
> >>>> On the other hand, msi_notify()/msix_notify() write to these vectors to
> >>>> signal the guest about an interrupt so we have to write correct vectors
> >>>> to the devices in order not to change every user of MSI/MSIX.
> >>>>
> >>>> The first aim is to support MSIX for virtio-pci on POWER. There is
> >>>> another patch for POWER coming which introduces a special memory region
> >>>> where MSI/MSIX vectors point to.
> >>>>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> ---
> >>>>  hw/msi.c  |   14 ++++++++++++++
> >>>>  hw/msi.h  |    1 +
> >>>>  hw/msix.c |   10 ++++++++++
> >>>>  hw/msix.h |    3 +++
> >>>>  4 files changed, 28 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/hw/msi.c b/hw/msi.c
> >>>> index 5d6ceb6..124878a 100644
> >>>> --- a/hw/msi.c
> >>>> +++ b/hw/msi.c
> >>>> @@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
> >>>>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> >>>>      return msi_nr_vectors(flags);
> >>>>  }
> >>>> +
> >>>> +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
> >>>> +{
> >>>> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
> >>>> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
> >>>> +
> >>>> +    if (msi64bit) {
> >>>> +        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
> >>>> +    } else {
> >>>> +        pci_set_long(dev->config + msi_address_lo_off(dev), address);
> >>>> +    }
> >>>> +    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
> >>>> +}
> >>>
> >>> Why not make it msi_set_message() and pass MSIMessage?  I'd be great if
> >>> you tossed in a msi_get_message() as well, I think we need it to be able
> >>> to do a kvm_irqchip_add_msi_route() with MSI.  Thanks,
> >>
> >>
> >> I am missing the point. What is that MSIMessage?
> >> It is just an address and data, making a struct from this is a bit too much :)
> >> 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,
> 
> Ok. Slowly :) What QEMU tree are you talking about? git, branch?
> There is neither MSIVectorUseNotifier nor MSIMessage in your or mine trees.

http://git.qemu.org/?p=qemu.git;f=hw/msi.h;hb=HEAD
http://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci.h;hb=HEAD

Very recent changesets by Jan, see 14de9bab & 2cdfe53c.  If I can get my
msix changes in, I'll push an updated tree for vfio that makes use of
these.  Thanks,

Alex

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

[Jan Kiszka]

On 2012-06-14 07:17, Alexey Kardashevskiy wrote:
> On 14/06/12 14:56, Alex Williamson wrote:
>> On Thu, 2012-06-14 at 14:31 +1000, Alexey Kardashevskiy wrote:
>>> Normally QEMU expects the guest to initialize MSI/MSIX vectors.
>>> However on POWER the guest uses RTAS subsystem to configure MSI/MSIX and
>>> does not write these vectors to device's config space or MSIX BAR.
>>>
>>> On the other hand, msi_notify()/msix_notify() write to these vectors to
>>> signal the guest about an interrupt so we have to write correct vectors
>>> to the devices in order not to change every user of MSI/MSIX.
>>>
>>> The first aim is to support MSIX for virtio-pci on POWER. There is
>>> another patch for POWER coming which introduces a special memory region
>>> where MSI/MSIX vectors point to.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>  hw/msi.c  |   14 ++++++++++++++
>>>  hw/msi.h  |    1 +
>>>  hw/msix.c |   10 ++++++++++
>>>  hw/msix.h |    3 +++
>>>  4 files changed, 28 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/msi.c b/hw/msi.c
>>> index 5d6ceb6..124878a 100644
>>> --- a/hw/msi.c
>>> +++ b/hw/msi.c
>>> @@ -358,3 +358,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
>>>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>>      return msi_nr_vectors(flags);
>>>  }
>>> +
>>> +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
>>> +{
>>> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>>> +
>>> +    if (msi64bit) {
>>> +        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
>>> +    } else {
>>> +        pci_set_long(dev->config + msi_address_lo_off(dev), address);
>>> +    }
>>> +    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
>>> +}
>>
>> Why not make it msi_set_message() and pass MSIMessage?  I'd be great if
>> you tossed in a msi_get_message() as well, I think we need it to be able
>> to do a kvm_irqchip_add_msi_route() with MSI.  Thanks,
> 
> 
> I am missing the point. What is that MSIMessage?
> It is just an address and data, making a struct from this is a bit too much :)

This is about consistent APIs. In practice (assembly), MSIMessage will
make no difference from open-coded address/data tuples, but it is
cleaner to pass around. Please follow the existing patterns.

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

The kvm_irqchip_* API come into play when you implement some interrupt
controller models in the kernel for performance reasons. We have this on
x86, we will see it on Book3E and ARM soon. You are targeting Book3S,
right? Not sure what the plans are there.

And, yes, msi_get_message() will be needed soon as well, at latest when
I push vector notifiers for classic MSI. If you have a need for reading
the message back, please add this helper already.

Jan

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

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

[Alexey Kardashevskiy]

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


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

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

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

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

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


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


-- 
Alexey

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

[Jan Kiszka]

On 2012-06-21 08:46, Alexey Kardashevskiy wrote:
> 
> Ok, another try. Is it any better now? :)

No - posted the old version accidentally?

Jan

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


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

[Qemu-devel] [PATCH] msi/msix: added public API to set/get MSI message address, and data

[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 <aik@ozlabs.ru>
---
 hw/msi.c  |   29 +++++++++++++++++++++++++++++
 hw/msi.h  |    2 ++
 hw/msix.c |   11 ++++++++++-
 hw/msix.h |    3 +++
 4 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/hw/msi.c b/hw/msi.c
index 5233204..9ad84a4 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -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);
 }
 
+MSIMessage msi_get_message(PCIDevice *dev)
+{
+    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));
+
+    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) &&
diff --git a/hw/msi.h b/hw/msi.h
index 75747ab..4b0f4f8 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -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);
diff --git a/hw/msix.c b/hw/msix.c
index ded3c55..9e8d8bb 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -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.
diff --git a/hw/msix.h b/hw/msix.h
index 50aee82..3374cf8 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -4,6 +4,9 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+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);
-- 
1.7.10



On 21/06/12 16:53, Jan Kiszka wrote:
> On 2012-06-21 08:46, Alexey Kardashevskiy wrote:
>>
>> Ok, another try. Is it any better now? :)
> 
> No - posted the old version accidentally?
> 
> Jan
> 
>>
>>
>> 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 <aik@ozlabs.ru>
>> ---
>>  hw/msi.c  |   14 ++++++++++++++
>>  hw/msi.h  |    1 +
>>  hw/msix.c |    8 ++++++++
>>  hw/msix.h |    3 +++
>>  4 files changed, 26 insertions(+)
>>
>> diff --git a/hw/msi.c b/hw/msi.c
>> index 5233204..c7b3e6a 100644
>> --- a/hw/msi.c
>> +++ b/hw/msi.c
>> @@ -363,3 +363,17 @@ unsigned int msi_nr_vectors_allocated(const PCIDevice *dev)
>>      uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>>      return msi_nr_vectors(flags);
>>  }
>> +
>> +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data)
>> +{
>> +    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
>> +    bool msi64bit = flags & PCI_MSI_FLAGS_64BIT;
>> +
>> +    if (msi64bit) {
>> +        pci_set_quad(dev->config + msi_address_lo_off(dev), address);
>> +    } else {
>> +        pci_set_long(dev->config + msi_address_lo_off(dev), address);
>> +    }
>> +    pci_set_word(dev->config + msi_data_off(dev, msi64bit), data);
>> +}
>> +
>> diff --git a/hw/msi.h b/hw/msi.h
>> index 75747ab..353386e 100644
>> --- a/hw/msi.h
>> +++ b/hw/msi.h
>> @@ -39,6 +39,7 @@ void msi_reset(PCIDevice *dev);
>>  void msi_notify(PCIDevice *dev, unsigned int vector);
>>  void msi_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len);
>>  unsigned int msi_nr_vectors_allocated(const PCIDevice *dev);
>> +void msi_set_address_data(PCIDevice *dev, uint64_t address, uint16_t data);
>>  
>>  static inline bool msi_present(const PCIDevice *dev)
>>  {
>> diff --git a/hw/msix.c b/hw/msix.c
>> index ded3c55..08e773d 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -526,3 +526,11 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
>>      dev->msix_vector_use_notifier = NULL;
>>      dev->msix_vector_release_notifier = NULL;
>>  }
>> +void msix_set_address_data(PCIDevice *dev, int vector,
>> +                           uint64_t address, uint32_t data)
>> +{
>> +    uint8_t *table_entry = dev->msix_table_page + vector * PCI_MSIX_ENTRY_SIZE;
>> +    pci_set_quad(table_entry + PCI_MSIX_ENTRY_LOWER_ADDR, address);
>> +    pci_set_long(table_entry + PCI_MSIX_ENTRY_DATA, data);
>> +    table_entry[PCI_MSIX_ENTRY_VECTOR_CTRL] &= ~PCI_MSIX_ENTRY_CTRL_MASKBIT;
>> +}
>> diff --git a/hw/msix.h b/hw/msix.h
>> index 50aee82..901f101 100644
>> --- a/hw/msix.h
>> +++ b/hw/msix.h
>> @@ -35,4 +35,7 @@ int msix_set_vector_notifiers(PCIDevice *dev,
>>                                MSIVectorUseNotifier use_notifier,
>>                                MSIVectorReleaseNotifier release_notifier);
>>  void msix_unset_vector_notifiers(PCIDevice *dev);
>> +void msix_set_address_data(PCIDevice *dev, int vector,
>> +                           uint64_t address, uint32_t data);
>> +
>>  #endif
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added public API to set/get MSI message address, and data

[Jan Kiszka]

On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
> 
> 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 <aik@ozlabs.ru>
> ---
>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
>  hw/msi.h  |    2 ++
>  hw/msix.c |   11 ++++++++++-
>  hw/msix.h |    3 +++
>  4 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/msi.c b/hw/msi.c
> index 5233204..9ad84a4 100644
> --- a/hw/msi.c
> +++ b/hw/msi.c
> @@ -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);
>  }
>  
> +MSIMessage msi_get_message(PCIDevice *dev)

MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)

> +{
> +    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.

> +
> +    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) &&
> diff --git a/hw/msi.h b/hw/msi.h
> index 75747ab..4b0f4f8 100644
> --- a/hw/msi.h
> +++ b/hw/msi.h
> @@ -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);
> diff --git a/hw/msix.c b/hw/msix.c
> index ded3c55..9e8d8bb 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -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.
> diff --git a/hw/msix.h b/hw/msix.h
> index 50aee82..3374cf8 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -4,6 +4,9 @@
>  #include "qemu-common.h"
>  #include "pci.h"
>  
> +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] ...".

Jan

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

Re: [Qemu-devel] [PATCH] msi/msix: added public API to set/get MSI message address, and data

[Alexey Kardashevskiy]

On 21/06/12 17:39, Jan Kiszka wrote:
> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
>>
>> 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 <aik@ozlabs.ru>
>> ---
>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
>>  hw/msi.h  |    2 ++
>>  hw/msix.c |   11 ++++++++++-
>>  hw/msix.h |    3 +++
>>  4 files changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/msi.c b/hw/msi.c
>> index 5233204..9ad84a4 100644
>> --- a/hw/msi.c
>> +++ b/hw/msi.c
>> @@ -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);
>>  }
>>  
>> +MSIMessage msi_get_message(PCIDevice *dev)
> 
> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)


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

> 
>> +{
>> +    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.


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?


> 
>> +
>> +    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) &&
>> diff --git a/hw/msi.h b/hw/msi.h
>> index 75747ab..4b0f4f8 100644
>> --- a/hw/msi.h
>> +++ b/hw/msi.h
>> @@ -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);
>> diff --git a/hw/msix.c b/hw/msix.c
>> index ded3c55..9e8d8bb 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -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.
>> diff --git a/hw/msix.h b/hw/msix.h
>> index 50aee82..3374cf8 100644
>> --- a/hw/msix.h
>> +++ b/hw/msix.h
>> @@ -4,6 +4,9 @@
>>  #include "qemu-common.h"
>>  #include "pci.h"
>>  
>> +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] ...".

In general you're right.
I just kept in mind that this patch will be rejected anyway.


-- 
Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added public API to set/get MSI message address, and data

[Jan Kiszka]

On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
> On 21/06/12 17:39, Jan Kiszka wrote:
>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
>>>
>>> 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 <aik@ozlabs.ru>
>>> ---
>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
>>>  hw/msi.h  |    2 ++
>>>  hw/msix.c |   11 ++++++++++-
>>>  hw/msix.h |    3 +++
>>>  4 files changed, 44 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/msi.c b/hw/msi.c
>>> index 5233204..9ad84a4 100644
>>> --- a/hw/msi.c
>>> +++ b/hw/msi.c
>>> @@ -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);
>>>  }
>>>  
>>> +MSIMessage msi_get_message(PCIDevice *dev)
>>
>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
> 
> 
> Who/how/why is going to calculate the vector here?
> 
>>
>>> +{
>>> +    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.
> 
> 
> 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.

Jan

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

Re: [Qemu-devel] [PATCH] msi/msix: added public API to set/get MSI message address, and data

[Alexey Kardashevskiy]

On 21/06/12 20:38, Jan Kiszka wrote:
> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
>> On 21/06/12 17:39, Jan Kiszka wrote:
>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
>>>>
>>>> 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 <aik@ozlabs.ru>
>>>> ---
>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
>>>>  hw/msi.h  |    2 ++
>>>>  hw/msix.c |   11 ++++++++++-
>>>>  hw/msix.h |    3 +++
>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>> index 5233204..9ad84a4 100644
>>>> --- a/hw/msi.c
>>>> +++ b/hw/msi.c
>>>> @@ -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);
>>>>  }
>>>>  
>>>> +MSIMessage msi_get_message(PCIDevice *dev)
>>>
>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
>>
>>
>> Who/how/why is going to calculate the vector here?
>>
>>>
>>>> +{
>>>> +    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.
>>
>>
>> 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.

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.


-- 
Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added public API to set/get MSI message address, and data

[Jan Kiszka]

On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
> On 21/06/12 20:38, Jan Kiszka wrote:
>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
>>> On 21/06/12 17:39, Jan Kiszka wrote:
>>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
>>>>>
>>>>> 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 <aik@ozlabs.ru>
>>>>> ---
>>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
>>>>>  hw/msi.h  |    2 ++
>>>>>  hw/msix.c |   11 ++++++++++-
>>>>>  hw/msix.h |    3 +++
>>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>>> index 5233204..9ad84a4 100644
>>>>> --- a/hw/msi.c
>>>>> +++ b/hw/msi.c
>>>>> @@ -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);
>>>>>  }
>>>>>  
>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
>>>>
>>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
>>>
>>>
>>> Who/how/why is going to calculate the vector here?
>>>
>>>>
>>>>> +{
>>>>> +    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.
>>>
>>>
>>> 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.
> 
> 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.

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

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

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

[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 <aik@ozlabs.ru>
---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

diff --git a/hw/msi.c b/hw/msi.c
index 5233204..cc6102f 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -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) &&
diff --git a/hw/msi.h b/hw/msi.h
index 75747ab..6ec1f99 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -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);
diff --git a/hw/msix.c b/hw/msix.c
index ded3c55..5f7d6d3 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -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.
diff --git a/hw/msix.h b/hw/msix.h
index 50aee82..26a437e 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -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);
-- 
1.7.10

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






On 21/06/12 20:56, Jan Kiszka wrote:
> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
>> On 21/06/12 20:38, Jan Kiszka wrote:
>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
>>>> On 21/06/12 17:39, Jan Kiszka wrote:
>>>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
>>>>>>
>>>>>> 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 <aik@ozlabs.ru>
>>>>>> ---
>>>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
>>>>>>  hw/msi.h  |    2 ++
>>>>>>  hw/msix.c |   11 ++++++++++-
>>>>>>  hw/msix.h |    3 +++
>>>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>>>> index 5233204..9ad84a4 100644
>>>>>> --- a/hw/msi.c
>>>>>> +++ b/hw/msi.c
>>>>>> @@ -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);
>>>>>>  }
>>>>>>  
>>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
>>>>>
>>>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
>>>>
>>>>
>>>> Who/how/why is going to calculate the vector here?
>>>>
>>>>>
>>>>>> +{
>>>>>> +    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.
>>>>
>>>>
>>>> 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.
>>
>> 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.
> 
> 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
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Jan Kiszka]

On 2012-06-21 13:39, Alexey Kardashevskiy wrote:
> 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 <aik@ozlabs.ru>
> ---
>  hw/msi.c  |   13 +++++++++++++
>  hw/msi.h  |    1 +
>  hw/msix.c |    9 +++++++++
>  hw/msix.h |    2 ++
>  4 files changed, 25 insertions(+)
> 
> diff --git a/hw/msi.c b/hw/msi.c
> index 5233204..cc6102f 100644
> --- a/hw/msi.c
> +++ b/hw/msi.c
> @@ -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) &&
> diff --git a/hw/msi.h b/hw/msi.h
> index 75747ab..6ec1f99 100644
> --- a/hw/msi.h
> +++ b/hw/msi.h
> @@ -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);
> diff --git a/hw/msix.c b/hw/msix.c
> index ded3c55..5f7d6d3 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -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.
> diff --git a/hw/msix.h b/hw/msix.h
> index 50aee82..26a437e 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -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);
> 

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

> -- 
> 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.

Jan

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

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Alexey Kardashevskiy]

On 21/06/12 21:49, Jan Kiszka wrote:
> On 2012-06-21 13:39, Alexey Kardashevskiy wrote:
>> 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 <aik@ozlabs.ru>
>> ---
>>  hw/msi.c  |   13 +++++++++++++
>>  hw/msi.h  |    1 +
>>  hw/msix.c |    9 +++++++++
>>  hw/msix.h |    2 ++
>>  4 files changed, 25 insertions(+)
>>
>> diff --git a/hw/msi.c b/hw/msi.c
>> index 5233204..cc6102f 100644
>> --- a/hw/msi.c
>> +++ b/hw/msi.c
>> @@ -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) &&
>> diff --git a/hw/msi.h b/hw/msi.h
>> index 75747ab..6ec1f99 100644
>> --- a/hw/msi.h
>> +++ b/hw/msi.h
>> @@ -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);
>> diff --git a/hw/msix.c b/hw/msix.c
>> index ded3c55..5f7d6d3 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -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.
>> diff --git a/hw/msix.h b/hw/msix.h
>> index 50aee82..26a437e 100644
>> --- a/hw/msix.h
>> +++ b/hw/msix.h
>> @@ -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);
>>
> 
> Interface looks good as fas as I can tell (can't asses the POWER need
> for clearing the mask bit on msix_set_message).


I do not know exactly how x86 works (who/how allocates addresses for MSI/MSIX). On POWER at the
moment I did the following thing in QEMU:

- registered memory_region_init_io at some big address which the guest won't use, it is just for QEMU
- put address from the previous step to the MSIX BAR via msix_set_message() when msi is being configured
- then the sequence looks like:
	- vfio_msi_interrupt() calls msix_notify()
	- msix_notify() checks if it is masked via msix_is_masked() - and here PCI_MSIX_ENTRY_CTRL_MASKBIT
must be unset
	- stl_le_phys() - here I get a notification in my MemoryRegionOps::write() and do qemu_irq_pulse()

2 reasons to do that:
1) I did not have to change either msix or vfio - cool for submitting patches;
2) neither POWER guest or qemu changes the msi or msix PCI config (it is done by different mechanism
called RTAS), so I have to do this myself to support 1) and I do not have to care about someone
breaking my settings


>> -- 
>> 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.

Ahhh. Confused end-of-message with end-of-patch. I'll repost it.



-- 
Alexey

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

[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 <aik@ozlabs.ru>
---
 hw/msi.c  |   13 +++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |    9 +++++++++
 hw/msix.h |    2 ++
 4 files changed, 25 insertions(+)

diff --git a/hw/msi.c b/hw/msi.c
index 5233204..cc6102f 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -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) &&
diff --git a/hw/msi.h b/hw/msi.h
index 75747ab..6ec1f99 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -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);
diff --git a/hw/msix.c b/hw/msix.c
index ded3c55..5f7d6d3 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -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.
diff --git a/hw/msix.h b/hw/msix.h
index 50aee82..26a437e 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -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);
-- 
1.7.10

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Alexey Kardashevskiy]

Ping?


On 22/06/12 11:15, Alexey Kardashevskiy wrote:
> 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 <aik@ozlabs.ru>
> ---
>  hw/msi.c  |   13 +++++++++++++
>  hw/msi.h  |    1 +
>  hw/msix.c |    9 +++++++++
>  hw/msix.h |    2 ++
>  4 files changed, 25 insertions(+)
> 
> diff --git a/hw/msi.c b/hw/msi.c
> index 5233204..cc6102f 100644
> --- a/hw/msi.c
> +++ b/hw/msi.c
> @@ -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) &&
> diff --git a/hw/msi.h b/hw/msi.h
> index 75747ab..6ec1f99 100644
> --- a/hw/msi.h
> +++ b/hw/msi.h
> @@ -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);
> diff --git a/hw/msix.c b/hw/msix.c
> index ded3c55..5f7d6d3 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -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.
> diff --git a/hw/msix.h b/hw/msix.h
> index 50aee82..26a437e 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -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);


-- 
Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Jan Kiszka]

On 2012-07-02 06:28, Alexey Kardashevskiy wrote:
> Ping?
> 
> 
> On 22/06/12 11:15, Alexey Kardashevskiy wrote:
>> 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 <aik@ozlabs.ru>
>> ---
>>  hw/msi.c  |   13 +++++++++++++
>>  hw/msi.h  |    1 +
>>  hw/msix.c |    9 +++++++++
>>  hw/msix.h |    2 ++
>>  4 files changed, 25 insertions(+)
>>
>> diff --git a/hw/msi.c b/hw/msi.c
>> index 5233204..cc6102f 100644
>> --- a/hw/msi.c
>> +++ b/hw/msi.c
>> @@ -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) &&
>> diff --git a/hw/msi.h b/hw/msi.h
>> index 75747ab..6ec1f99 100644
>> --- a/hw/msi.h
>> +++ b/hw/msi.h
>> @@ -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);
>> diff --git a/hw/msix.c b/hw/msix.c
>> index ded3c55..5f7d6d3 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -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.
>> diff --git a/hw/msix.h b/hw/msix.h
>> index 50aee82..26a437e 100644
>> --- a/hw/msix.h
>> +++ b/hw/msix.h
>> @@ -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);
> 
> 

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Alexander Graf]

On 02.07.2012, at 09:24, Jan Kiszka wrote:

> On 2012-07-02 06:28, Alexey Kardashevskiy wrote:
>> Ping?
>> 
>> 
>> On 22/06/12 11:15, Alexey Kardashevskiy wrote:
>>> 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 <aik@ozlabs.ru>
>>> ---
>>> hw/msi.c  |   13 +++++++++++++
>>> hw/msi.h  |    1 +
>>> hw/msix.c |    9 +++++++++
>>> hw/msix.h |    2 ++
>>> 4 files changed, 25 insertions(+)
>>> 
>>> diff --git a/hw/msi.c b/hw/msi.c
>>> index 5233204..cc6102f 100644
>>> --- a/hw/msi.c
>>> +++ b/hw/msi.c
>>> @@ -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) &&
>>> diff --git a/hw/msi.h b/hw/msi.h
>>> index 75747ab..6ec1f99 100644
>>> --- a/hw/msi.h
>>> +++ b/hw/msi.h
>>> @@ -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);
>>> diff --git a/hw/msix.c b/hw/msix.c
>>> index ded3c55..5f7d6d3 100644
>>> --- a/hw/msix.c
>>> +++ b/hw/msix.c
>>> @@ -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.
>>> diff --git a/hw/msix.h b/hw/msix.h
>>> index 50aee82..26a437e 100644
>>> --- a/hw/msix.h
>>> +++ b/hw/msix.h
>>> @@ -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);
>> 
>> 
> 
> Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

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


Alex

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Jan Kiszka]

On 2012-07-06 17:36, Alexander Graf wrote:
> 
> On 02.07.2012, at 09:24, Jan Kiszka wrote:
> 
>> On 2012-07-02 06:28, Alexey Kardashevskiy wrote:
>>> Ping?
>>>
>>>
>>> On 22/06/12 11:15, Alexey Kardashevskiy wrote:
>>>> 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 <aik@ozlabs.ru>
>>>> ---
>>>> hw/msi.c  |   13 +++++++++++++
>>>> hw/msi.h  |    1 +
>>>> hw/msix.c |    9 +++++++++
>>>> hw/msix.h |    2 ++
>>>> 4 files changed, 25 insertions(+)
>>>>
>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>> index 5233204..cc6102f 100644
>>>> --- a/hw/msi.c
>>>> +++ b/hw/msi.c
>>>> @@ -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) &&
>>>> diff --git a/hw/msi.h b/hw/msi.h
>>>> index 75747ab..6ec1f99 100644
>>>> --- a/hw/msi.h
>>>> +++ b/hw/msi.h
>>>> @@ -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);
>>>> diff --git a/hw/msix.c b/hw/msix.c
>>>> index ded3c55..5f7d6d3 100644
>>>> --- a/hw/msix.c
>>>> +++ b/hw/msix.c
>>>> @@ -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.
>>>> diff --git a/hw/msix.h b/hw/msix.h
>>>> index 50aee82..26a437e 100644
>>>> --- a/hw/msix.h
>>>> +++ b/hw/msix.h
>>>> @@ -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);
>>>
>>>
>>
>> Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Alexander Graf]

On 06.07.2012, at 17:58, Jan Kiszka wrote:

> On 2012-07-06 17:36, Alexander Graf wrote:
>> 
>> On 02.07.2012, at 09:24, Jan Kiszka wrote:
>> 
>>> On 2012-07-02 06:28, Alexey Kardashevskiy wrote:
>>>> Ping?
>>>> 
>>>> 
>>>> On 22/06/12 11:15, Alexey Kardashevskiy wrote:
>>>>> 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 <aik@ozlabs.ru>
>>>>> ---
>>>>> hw/msi.c  |   13 +++++++++++++
>>>>> hw/msi.h  |    1 +
>>>>> hw/msix.c |    9 +++++++++
>>>>> hw/msix.h |    2 ++
>>>>> 4 files changed, 25 insertions(+)
>>>>> 
>>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>>> index 5233204..cc6102f 100644
>>>>> --- a/hw/msi.c
>>>>> +++ b/hw/msi.c
>>>>> @@ -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) &&
>>>>> diff --git a/hw/msi.h b/hw/msi.h
>>>>> index 75747ab..6ec1f99 100644
>>>>> --- a/hw/msi.h
>>>>> +++ b/hw/msi.h
>>>>> @@ -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);
>>>>> diff --git a/hw/msix.c b/hw/msix.c
>>>>> index ded3c55..5f7d6d3 100644
>>>>> --- a/hw/msix.c
>>>>> +++ b/hw/msix.c
>>>>> @@ -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.
>>>>> diff --git a/hw/msix.h b/hw/msix.h
>>>>> index 50aee82..26a437e 100644
>>>>> --- a/hw/msix.h
>>>>> +++ b/hw/msix.h
>>>>> @@ -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);
>>>> 
>>>> 
>>> 
>>> Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
>> 
>> 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.

Michael?


Alex

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Michael S. Tsirkin]

On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey Kardashevskiy wrote:
> 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 <aik@ozlabs.ru>

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!

> ---
>  hw/msi.c  |   13 +++++++++++++
>  hw/msi.h  |    1 +
>  hw/msix.c |    9 +++++++++
>  hw/msix.h |    2 ++
>  4 files changed, 25 insertions(+)
> 
> diff --git a/hw/msi.c b/hw/msi.c
> index 5233204..cc6102f 100644
> --- a/hw/msi.c
> +++ b/hw/msi.c
> @@ -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);
> +}
> +

Please add documentation. Something like

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

>  bool msi_enabled(const PCIDevice *dev)
>  {
>      return msi_present(dev) &&
> diff --git a/hw/msi.h b/hw/msi.h
> index 75747ab..6ec1f99 100644
> --- a/hw/msi.h
> +++ b/hw/msi.h
> @@ -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);
> diff --git a/hw/msix.c b/hw/msix.c
> index ded3c55..5f7d6d3 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -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.
> diff --git a/hw/msix.h b/hw/msix.h
> index 50aee82..26a437e 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -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);
> -- 
> 1.7.10
> 
> ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)
> 
> 
> 
> 
> 
> 
> On 21/06/12 20:56, Jan Kiszka wrote:
> > On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
> >> On 21/06/12 20:38, Jan Kiszka wrote:
> >>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
> >>>> On 21/06/12 17:39, Jan Kiszka wrote:
> >>>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
> >>>>>>
> >>>>>> 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 <aik@ozlabs.ru>
> >>>>>> ---
> >>>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
> >>>>>>  hw/msi.h  |    2 ++
> >>>>>>  hw/msix.c |   11 ++++++++++-
> >>>>>>  hw/msix.h |    3 +++
> >>>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hw/msi.c b/hw/msi.c
> >>>>>> index 5233204..9ad84a4 100644
> >>>>>> --- a/hw/msi.c
> >>>>>> +++ b/hw/msi.c
> >>>>>> @@ -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);
> >>>>>>  }
> >>>>>>  
> >>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
> >>>>>
> >>>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
> >>>>
> >>>>
> >>>> Who/how/why is going to calculate the vector here?
> >>>>
> >>>>>
> >>>>>> +{
> >>>>>> +    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.
> >>>>
> >>>>
> >>>> 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.
> >>
> >> 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.
> > 
> > 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
> > 
> 
> 
> -- 
> Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Alexey Kardashevskiy]

On 18/07/12 22:43, Michael S. Tsirkin wrote:
> On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey Kardashevskiy wrote:
>> 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 <aik@ozlabs.ru>
> 
> 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.


> 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.


> Can't we just fix guest drivers to program the vectors properly?
> 
> Also pls address the comment below.

Comment below.

> Thanks!
> 
>> ---
>>  hw/msi.c  |   13 +++++++++++++
>>  hw/msi.h  |    1 +
>>  hw/msix.c |    9 +++++++++
>>  hw/msix.h |    2 ++
>>  4 files changed, 25 insertions(+)
>>
>> diff --git a/hw/msi.c b/hw/msi.c
>> index 5233204..cc6102f 100644
>> --- a/hw/msi.c
>> +++ b/hw/msi.c
>> @@ -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);
>> +}
>> +
> 
> Please add documentation. Something like
> 
> /*
>  * 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 :)



>>  bool msi_enabled(const PCIDevice *dev)
>>  {
>>      return msi_present(dev) &&
>> diff --git a/hw/msi.h b/hw/msi.h
>> index 75747ab..6ec1f99 100644
>> --- a/hw/msi.h
>> +++ b/hw/msi.h
>> @@ -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);
>> diff --git a/hw/msix.c b/hw/msix.c
>> index ded3c55..5f7d6d3 100644
>> --- a/hw/msix.c
>> +++ b/hw/msix.c
>> @@ -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.
>> diff --git a/hw/msix.h b/hw/msix.h
>> index 50aee82..26a437e 100644
>> --- a/hw/msix.h
>> +++ b/hw/msix.h
>> @@ -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);
>> -- 
>> 1.7.10
>>
>> ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)
>>
>>
>>
>>
>>
>>
>> On 21/06/12 20:56, Jan Kiszka wrote:
>>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
>>>> On 21/06/12 20:38, Jan Kiszka wrote:
>>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
>>>>>> On 21/06/12 17:39, Jan Kiszka wrote:
>>>>>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
>>>>>>>>
>>>>>>>> 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 <aik@ozlabs.ru>
>>>>>>>> ---
>>>>>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
>>>>>>>>  hw/msi.h  |    2 ++
>>>>>>>>  hw/msix.c |   11 ++++++++++-
>>>>>>>>  hw/msix.h |    3 +++
>>>>>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>>>>>> index 5233204..9ad84a4 100644
>>>>>>>> --- a/hw/msi.c
>>>>>>>> +++ b/hw/msi.c
>>>>>>>> @@ -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);
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
>>>>>>>
>>>>>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
>>>>>>
>>>>>>
>>>>>> Who/how/why is going to calculate the vector here?
>>>>>>
>>>>>>>
>>>>>>>> +{
>>>>>>>> +    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.
>>>>>>
>>>>>>
>>>>>> 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.
>>>>
>>>> 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.
>>>
>>> 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.



-- 
Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Michael S. Tsirkin]

On Wed, Jul 18, 2012 at 11:17:12PM +1000, Alexey Kardashevskiy wrote:
> On 18/07/12 22:43, Michael S. Tsirkin wrote:
> > On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey Kardashevskiy wrote:
> >> 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 <aik@ozlabs.ru>
> > 
> > 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.

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

> > 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.

> > Can't we just fix guest drivers to program the vectors properly?
> > 
> > Also pls address the comment below.
> 
> Comment below.
> 
> > Thanks!
> > 
> >> ---
> >>  hw/msi.c  |   13 +++++++++++++
> >>  hw/msi.h  |    1 +
> >>  hw/msix.c |    9 +++++++++
> >>  hw/msix.h |    2 ++
> >>  4 files changed, 25 insertions(+)
> >>
> >> diff --git a/hw/msi.c b/hw/msi.c
> >> index 5233204..cc6102f 100644
> >> --- a/hw/msi.c
> >> +++ b/hw/msi.c
> >> @@ -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);
> >> +}
> >> +
> > 
> > Please add documentation. Something like
> > 
> > /*
> >  * 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.

> >>  bool msi_enabled(const PCIDevice *dev)
> >>  {
> >>      return msi_present(dev) &&
> >> diff --git a/hw/msi.h b/hw/msi.h
> >> index 75747ab..6ec1f99 100644
> >> --- a/hw/msi.h
> >> +++ b/hw/msi.h
> >> @@ -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);
> >> diff --git a/hw/msix.c b/hw/msix.c
> >> index ded3c55..5f7d6d3 100644
> >> --- a/hw/msix.c
> >> +++ b/hw/msix.c
> >> @@ -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.
> >> diff --git a/hw/msix.h b/hw/msix.h
> >> index 50aee82..26a437e 100644
> >> --- a/hw/msix.h
> >> +++ b/hw/msix.h
> >> @@ -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);
> >> -- 
> >> 1.7.10
> >>
> >> ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)
> >>
> >>
> >>
> >>
> >>
> >>
> >> On 21/06/12 20:56, Jan Kiszka wrote:
> >>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
> >>>> On 21/06/12 20:38, Jan Kiszka wrote:
> >>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
> >>>>>> On 21/06/12 17:39, Jan Kiszka wrote:
> >>>>>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
> >>>>>>>>
> >>>>>>>> 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 <aik@ozlabs.ru>
> >>>>>>>> ---
> >>>>>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
> >>>>>>>>  hw/msi.h  |    2 ++
> >>>>>>>>  hw/msix.c |   11 ++++++++++-
> >>>>>>>>  hw/msix.h |    3 +++
> >>>>>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/hw/msi.c b/hw/msi.c
> >>>>>>>> index 5233204..9ad84a4 100644
> >>>>>>>> --- a/hw/msi.c
> >>>>>>>> +++ b/hw/msi.c
> >>>>>>>> @@ -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);
> >>>>>>>>  }
> >>>>>>>>  
> >>>>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
> >>>>>>>
> >>>>>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
> >>>>>>
> >>>>>>
> >>>>>> Who/how/why is going to calculate the vector here?
> >>>>>>
> >>>>>>>
> >>>>>>>> +{
> >>>>>>>> +    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.
> >>>>>>
> >>>>>>
> >>>>>> 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.
> >>>>
> >>>> 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.
> >>>
> >>> 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.
> 
> 
> 
> -- 
> Alexey
> 

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Alexey Kardashevskiy]

On 19/07/12 01:23, Michael S. Tsirkin wrote:
> On Wed, Jul 18, 2012 at 11:17:12PM +1000, Alexey Kardashevskiy wrote:
>> On 18/07/12 22:43, Michael S. Tsirkin wrote:
>>> On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey Kardashevskiy wrote:
>>>> 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 <aik@ozlabs.ru>
>>>
>>> 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.
> 
> 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.


>>> 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.
> 
>>> Can't we just fix guest drivers to program the vectors properly?
>>>
>>> Also pls address the comment below.
>>
>> Comment below.
>>
>>> Thanks!
>>>
>>>> ---
>>>>  hw/msi.c  |   13 +++++++++++++
>>>>  hw/msi.h  |    1 +
>>>>  hw/msix.c |    9 +++++++++
>>>>  hw/msix.h |    2 ++
>>>>  4 files changed, 25 insertions(+)
>>>>
>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>> index 5233204..cc6102f 100644
>>>> --- a/hw/msi.c
>>>> +++ b/hw/msi.c
>>>> @@ -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);
>>>> +}
>>>> +
>>>
>>> Please add documentation. Something like
>>>
>>> /*
>>>  * 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.


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.


>>>>  bool msi_enabled(const PCIDevice *dev)
>>>>  {
>>>>      return msi_present(dev) &&
>>>> diff --git a/hw/msi.h b/hw/msi.h
>>>> index 75747ab..6ec1f99 100644
>>>> --- a/hw/msi.h
>>>> +++ b/hw/msi.h
>>>> @@ -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);
>>>> diff --git a/hw/msix.c b/hw/msix.c
>>>> index ded3c55..5f7d6d3 100644
>>>> --- a/hw/msix.c
>>>> +++ b/hw/msix.c
>>>> @@ -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.
>>>> diff --git a/hw/msix.h b/hw/msix.h
>>>> index 50aee82..26a437e 100644
>>>> --- a/hw/msix.h
>>>> +++ b/hw/msix.h
>>>> @@ -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);
>>>> -- 
>>>> 1.7.10
>>>>
>>>> ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On 21/06/12 20:56, Jan Kiszka wrote:
>>>>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
>>>>>> On 21/06/12 20:38, Jan Kiszka wrote:
>>>>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
>>>>>>>> On 21/06/12 17:39, Jan Kiszka wrote:
>>>>>>>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
>>>>>>>>>>
>>>>>>>>>> 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 <aik@ozlabs.ru>
>>>>>>>>>> ---
>>>>>>>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
>>>>>>>>>>  hw/msi.h  |    2 ++
>>>>>>>>>>  hw/msix.c |   11 ++++++++++-
>>>>>>>>>>  hw/msix.h |    3 +++
>>>>>>>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>>>>>>>> index 5233204..9ad84a4 100644
>>>>>>>>>> --- a/hw/msi.c
>>>>>>>>>> +++ b/hw/msi.c
>>>>>>>>>> @@ -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);
>>>>>>>>>>  }
>>>>>>>>>>  
>>>>>>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
>>>>>>>>>
>>>>>>>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
>>>>>>>>
>>>>>>>>
>>>>>>>> Who/how/why is going to calculate the vector here?
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +{
>>>>>>>>>> +    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.
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> 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.
>>>>>
>>>>> 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.


-- 
Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Michael S. Tsirkin]

On Thu, Jul 19, 2012 at 10:32:40AM +1000, Alexey Kardashevskiy wrote:
> On 19/07/12 01:23, Michael S. Tsirkin wrote:
> > On Wed, Jul 18, 2012 at 11:17:12PM +1000, Alexey Kardashevskiy wrote:
> >> On 18/07/12 22:43, Michael S. Tsirkin wrote:
> >>> On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey Kardashevskiy wrote:
> >>>> 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 <aik@ozlabs.ru>
> >>>
> >>> 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.
> > 
> > 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.

> >>> 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.
> > 
> >>> Can't we just fix guest drivers to program the vectors properly?
> >>>
> >>> Also pls address the comment below.
> >>
> >> Comment below.
> >>
> >>> Thanks!
> >>>
> >>>> ---
> >>>>  hw/msi.c  |   13 +++++++++++++
> >>>>  hw/msi.h  |    1 +
> >>>>  hw/msix.c |    9 +++++++++
> >>>>  hw/msix.h |    2 ++
> >>>>  4 files changed, 25 insertions(+)
> >>>>
> >>>> diff --git a/hw/msi.c b/hw/msi.c
> >>>> index 5233204..cc6102f 100644
> >>>> --- a/hw/msi.c
> >>>> +++ b/hw/msi.c
> >>>> @@ -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);
> >>>> +}
> >>>> +
> >>>
> >>> Please add documentation. Something like
> >>>
> >>> /*
> >>>  * 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.
> 
> 
> 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 :)

> >>>>  bool msi_enabled(const PCIDevice *dev)
> >>>>  {
> >>>>      return msi_present(dev) &&
> >>>> diff --git a/hw/msi.h b/hw/msi.h
> >>>> index 75747ab..6ec1f99 100644
> >>>> --- a/hw/msi.h
> >>>> +++ b/hw/msi.h
> >>>> @@ -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);
> >>>> diff --git a/hw/msix.c b/hw/msix.c
> >>>> index ded3c55..5f7d6d3 100644
> >>>> --- a/hw/msix.c
> >>>> +++ b/hw/msix.c
> >>>> @@ -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.
> >>>> diff --git a/hw/msix.h b/hw/msix.h
> >>>> index 50aee82..26a437e 100644
> >>>> --- a/hw/msix.h
> >>>> +++ b/hw/msix.h
> >>>> @@ -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);
> >>>> -- 
> >>>> 1.7.10
> >>>>
> >>>> ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> On 21/06/12 20:56, Jan Kiszka wrote:
> >>>>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
> >>>>>> On 21/06/12 20:38, Jan Kiszka wrote:
> >>>>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
> >>>>>>>> On 21/06/12 17:39, Jan Kiszka wrote:
> >>>>>>>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
> >>>>>>>>>>
> >>>>>>>>>> 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 <aik@ozlabs.ru>
> >>>>>>>>>> ---
> >>>>>>>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
> >>>>>>>>>>  hw/msi.h  |    2 ++
> >>>>>>>>>>  hw/msix.c |   11 ++++++++++-
> >>>>>>>>>>  hw/msix.h |    3 +++
> >>>>>>>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/hw/msi.c b/hw/msi.c
> >>>>>>>>>> index 5233204..9ad84a4 100644
> >>>>>>>>>> --- a/hw/msi.c
> >>>>>>>>>> +++ b/hw/msi.c
> >>>>>>>>>> @@ -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);
> >>>>>>>>>>  }
> >>>>>>>>>>  
> >>>>>>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
> >>>>>>>>>
> >>>>>>>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Who/how/why is going to calculate the vector here?
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> +{
> >>>>>>>>>> +    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.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 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.
> >>>>>>
> >>>>>> 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.
> >>>>>
> >>>>> 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.
> 
> 
> -- 
> Alexey
> 

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Alexey Kardashevskiy]

One comment below.


On 19/07/12 19:27, Michael S. Tsirkin wrote:
> On Thu, Jul 19, 2012 at 10:32:40AM +1000, Alexey Kardashevskiy wrote:
>> On 19/07/12 01:23, Michael S. Tsirkin wrote:
>>> On Wed, Jul 18, 2012 at 11:17:12PM +1000, Alexey Kardashevskiy wrote:
>>>> On 18/07/12 22:43, Michael S. Tsirkin wrote:
>>>>> On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey Kardashevskiy wrote:
>>>>>> 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 <aik@ozlabs.ru>
>>>>>
>>>>> 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.
>>>
>>> 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.
> 
>>>>> 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.
>>>
>>>>> Can't we just fix guest drivers to program the vectors properly?
>>>>>
>>>>> Also pls address the comment below.
>>>>
>>>> Comment below.
>>>>
>>>>> Thanks!
>>>>>
>>>>>> ---
>>>>>>  hw/msi.c  |   13 +++++++++++++
>>>>>>  hw/msi.h  |    1 +
>>>>>>  hw/msix.c |    9 +++++++++
>>>>>>  hw/msix.h |    2 ++
>>>>>>  4 files changed, 25 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>>>> index 5233204..cc6102f 100644
>>>>>> --- a/hw/msi.c
>>>>>> +++ b/hw/msi.c
>>>>>> @@ -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);
>>>>>> +}
>>>>>> +
>>>>>
>>>>> Please add documentation. Something like
>>>>>
>>>>> /*
>>>>>  * 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.
>>
>>
>> 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 :)


It depends on how to look at it. Enabling MSI via the config space is also done via a special set of hypervisor calls (common and IBM-specific) so it is all hidden in one place - the system firmware, what is cool - no PHB drivers in the guest. Although MSI would not need any additional hypercall to init vectors (everything can be done via config space), there is MSI-X which stores vectors in BAR and there is no hypercall for BARs as they are simply memory mapped. This is I think why the firmware people (or phyp but it is probably the same) added IBM-specific MSI/MSIX config hypercalls.

And I do not quite understand why MSIX people could not use extended PCI config space which is 4096 bytes, quite a lot, enough to fit 256 vectors (have not seen a card which asked for more than 9 _per function_). If somebody really needs 2048, he may want 16384 as well (or any other crazy number), etc, so why did they put such a limit, it is a BAR, it is huge? :) A, offtopic anyway.


>>>>>>  bool msi_enabled(const PCIDevice *dev)
>>>>>>  {
>>>>>>      return msi_present(dev) &&
>>>>>> diff --git a/hw/msi.h b/hw/msi.h
>>>>>> index 75747ab..6ec1f99 100644
>>>>>> --- a/hw/msi.h
>>>>>> +++ b/hw/msi.h
>>>>>> @@ -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);
>>>>>> diff --git a/hw/msix.c b/hw/msix.c
>>>>>> index ded3c55..5f7d6d3 100644
>>>>>> --- a/hw/msix.c
>>>>>> +++ b/hw/msix.c
>>>>>> @@ -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.
>>>>>> diff --git a/hw/msix.h b/hw/msix.h
>>>>>> index 50aee82..26a437e 100644
>>>>>> --- a/hw/msix.h
>>>>>> +++ b/hw/msix.h
>>>>>> @@ -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);
>>>>>> -- 
>>>>>> 1.7.10
>>>>>>
>>>>>> ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 21/06/12 20:56, Jan Kiszka wrote:
>>>>>>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
>>>>>>>> On 21/06/12 20:38, Jan Kiszka wrote:
>>>>>>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
>>>>>>>>>> On 21/06/12 17:39, Jan Kiszka wrote:
>>>>>>>>>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> 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 <aik@ozlabs.ru>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
>>>>>>>>>>>>  hw/msi.h  |    2 ++
>>>>>>>>>>>>  hw/msix.c |   11 ++++++++++-
>>>>>>>>>>>>  hw/msix.h |    3 +++
>>>>>>>>>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/hw/msi.c b/hw/msi.c
>>>>>>>>>>>> index 5233204..9ad84a4 100644
>>>>>>>>>>>> --- a/hw/msi.c
>>>>>>>>>>>> +++ b/hw/msi.c
>>>>>>>>>>>> @@ -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);
>>>>>>>>>>>>  }
>>>>>>>>>>>>  
>>>>>>>>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
>>>>>>>>>>>
>>>>>>>>>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Who/how/why is going to calculate the vector here?
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    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.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> 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.


-- 
Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Michael S. Tsirkin]

On Fri, Jul 20, 2012 at 12:24:05AM +1000, Alexey Kardashevskiy wrote:
> One comment below.
> 
> 
> On 19/07/12 19:27, Michael S. Tsirkin wrote:
> > On Thu, Jul 19, 2012 at 10:32:40AM +1000, Alexey Kardashevskiy wrote:
> >> On 19/07/12 01:23, Michael S. Tsirkin wrote:
> >>> On Wed, Jul 18, 2012 at 11:17:12PM +1000, Alexey Kardashevskiy wrote:
> >>>> On 18/07/12 22:43, Michael S. Tsirkin wrote:
> >>>>> On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey Kardashevskiy wrote:
> >>>>>> 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 <aik@ozlabs.ru>
> >>>>>
> >>>>> 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.
> >>>
> >>> 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.
> > 
> >>>>> 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.
> >>>
> >>>>> Can't we just fix guest drivers to program the vectors properly?
> >>>>>
> >>>>> Also pls address the comment below.
> >>>>
> >>>> Comment below.
> >>>>
> >>>>> Thanks!
> >>>>>
> >>>>>> ---
> >>>>>>  hw/msi.c  |   13 +++++++++++++
> >>>>>>  hw/msi.h  |    1 +
> >>>>>>  hw/msix.c |    9 +++++++++
> >>>>>>  hw/msix.h |    2 ++
> >>>>>>  4 files changed, 25 insertions(+)
> >>>>>>
> >>>>>> diff --git a/hw/msi.c b/hw/msi.c
> >>>>>> index 5233204..cc6102f 100644
> >>>>>> --- a/hw/msi.c
> >>>>>> +++ b/hw/msi.c
> >>>>>> @@ -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);
> >>>>>> +}
> >>>>>> +
> >>>>>
> >>>>> Please add documentation. Something like
> >>>>>
> >>>>> /*
> >>>>>  * 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.
> >>
> >>
> >> 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 :)
> 
> 
> It depends on how to look at it. Enabling MSI via the config space is also done via a special set of hypervisor calls (common and IBM-specific) so it is all hidden in one place - the system firmware, what is cool - no PHB drivers in the guest. Although MSI would not need any additional hypercall to init vectors (everything can be done via config space), there is MSI-X which stores vectors in BAR and there is no hypercall for BARs as they are simply memory mapped. This is I think why the firmware people (or phyp but it is probably the same) added IBM-specific MSI/MSIX config hypercalls.

Well what's wrong with guest doing this through a memory mapped interface?

> And I do not quite understand why MSIX people could not use extended PCI config space which is 4096 bytes, quite a lot, enough to fit 256 vectors (have not seen a card which asked for more than 9 _per function_). If somebody really needs 2048, he may want 16384 as well (or any other crazy number), etc, so why did they put such a limit, it is a BAR, it is huge? :) A, offtopic anyway.


Well you have just described MSI, just don't use MSIX.

The motivation for MSIX was as follows:
PCI/PCI-X config space is not 4096 bytes, it is 256 bytes, and is very
crowded. You are thinking of PCI express. Config accesses are also
nonposted which means at most one must be in flight. This is not
appropriate for vector programming which needs to be done from multiple
CPUs in parallel.

Also offtopic, please try to avoid these super long lines in mail :).

> 
> 
> >>>>>>  bool msi_enabled(const PCIDevice *dev)
> >>>>>>  {
> >>>>>>      return msi_present(dev) &&
> >>>>>> diff --git a/hw/msi.h b/hw/msi.h
> >>>>>> index 75747ab..6ec1f99 100644
> >>>>>> --- a/hw/msi.h
> >>>>>> +++ b/hw/msi.h
> >>>>>> @@ -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);
> >>>>>> diff --git a/hw/msix.c b/hw/msix.c
> >>>>>> index ded3c55..5f7d6d3 100644
> >>>>>> --- a/hw/msix.c
> >>>>>> +++ b/hw/msix.c
> >>>>>> @@ -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.
> >>>>>> diff --git a/hw/msix.h b/hw/msix.h
> >>>>>> index 50aee82..26a437e 100644
> >>>>>> --- a/hw/msix.h
> >>>>>> +++ b/hw/msix.h
> >>>>>> @@ -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);
> >>>>>> -- 
> >>>>>> 1.7.10
> >>>>>>
> >>>>>> ps. double '-' and git version is an end-of-patch scissor as I read somewhere, cannot recall where exactly :)
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 21/06/12 20:56, Jan Kiszka wrote:
> >>>>>>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
> >>>>>>>> On 21/06/12 20:38, Jan Kiszka wrote:
> >>>>>>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
> >>>>>>>>>> On 21/06/12 17:39, Jan Kiszka wrote:
> >>>>>>>>>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> 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 <aik@ozlabs.ru>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
> >>>>>>>>>>>>  hw/msi.h  |    2 ++
> >>>>>>>>>>>>  hw/msix.c |   11 ++++++++++-
> >>>>>>>>>>>>  hw/msix.h |    3 +++
> >>>>>>>>>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
> >>>>>>>>>>>>
> >>>>>>>>>>>> diff --git a/hw/msi.c b/hw/msi.c
> >>>>>>>>>>>> index 5233204..9ad84a4 100644
> >>>>>>>>>>>> --- a/hw/msi.c
> >>>>>>>>>>>> +++ b/hw/msi.c
> >>>>>>>>>>>> @@ -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);
> >>>>>>>>>>>>  }
> >>>>>>>>>>>>  
> >>>>>>>>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
> >>>>>>>>>>>
> >>>>>>>>>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Who/how/why is going to calculate the vector here?
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>> +{
> >>>>>>>>>>>> +    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.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> 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.
> >>>>>>>>
> >>>>>>>> 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.
> >>>>>>>
> >>>>>>> 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.
> 
> 
> -- 
> Alexey
> 

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Alexey Kardashevskiy]

On 20/07/12 00:43, Michael S. Tsirkin wrote:
> On Fri, Jul 20, 2012 at 12:24:05AM +1000, Alexey Kardashevskiy wrote:
>> One comment below.
>> 
>> 
>> On 19/07/12 19:27, Michael S. Tsirkin wrote:
>>> On Thu, Jul 19, 2012 at 10:32:40AM +1000, Alexey Kardashevskiy
>>> wrote:
>>>> On 19/07/12 01:23, Michael S. Tsirkin wrote:
>>>>> On Wed, Jul 18, 2012 at 11:17:12PM +1000, Alexey Kardashevskiy
>>>>> wrote:
>>>>>> On 18/07/12 22:43, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey
>>>>>>> Kardashevskiy wrote:
>>>>>>>> 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 <aik@ozlabs.ru>
>>>>>>> 
>>>>>>> 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.
>>>>> 
>>>>> 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.
>>> 
>>>>>>> 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.
>>>>> 
>>>>>>> Can't we just fix guest drivers to program the vectors
>>>>>>> properly?
>>>>>>> 
>>>>>>> Also pls address the comment below.
>>>>>> 
>>>>>> Comment below.
>>>>>> 
>>>>>>> Thanks!
>>>>>>> 
>>>>>>>> --- hw/msi.c  |   13 +++++++++++++ hw/msi.h  |    1 + 
>>>>>>>> hw/msix.c |    9 +++++++++ hw/msix.h |    2 ++ 4 files
>>>>>>>> changed, 25 insertions(+)
>>>>>>>> 
>>>>>>>> diff --git a/hw/msi.c b/hw/msi.c index 5233204..cc6102f
>>>>>>>> 100644 --- a/hw/msi.c +++ b/hw/msi.c @@ -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); +} +
>>>>>>> 
>>>>>>> Please add documentation. Something like
>>>>>>> 
>>>>>>> /* * 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.
>>>> 
>>>> 
>>>> 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 :)
>> 
>> 
>> It depends on how to look at it. Enabling MSI via the config space is
>> also done via a special set of hypervisor calls (common and
>> IBM-specific) so it is all hidden in one place - the system firmware,
>> what is cool - no PHB drivers in the guest. Although MSI would not
>> need any additional hypercall to init vectors (everything can be done
>> via config space), there is MSI-X which stores vectors in BAR and
>> there is no hypercall for BARs as they are simply memory mapped. This
>> is I think why the firmware people (or phyp but it is probably the
>> same) added IBM-specific MSI/MSIX config hypercalls.
> 
> Well what's wrong with guest doing this through a memory mapped
> interface?


Should not guest allocate addresses and program PHB with them?
The idea was to hide PHB details in the system firmware, this is the point.


>> And I do not quite understand why MSIX people could not use extended
>> PCI config space which is 4096 bytes, quite a lot, enough to fit 256
>> vectors (have not seen a card which asked for more than 9 _per
>> function_). If somebody really needs 2048, he may want 16384 as well
>> (or any other crazy number), etc, so why did they put such a limit, it
>> is a BAR, it is huge? :) A, offtopic anyway.


> Well you have just described MSI, just don't use MSIX.
> 
> The motivation for MSIX was as follows: PCI/PCI-X config space is not
> 4096 bytes, it is 256 bytes, and is very crowded. You are thinking of
> PCI express. 

MSIX is PCIe feature, no?

> Config accesses are also nonposted which means at most one
> must be in flight. This is not appropriate for vector programming which
> needs to be done from multiple CPUs in parallel.

> Also offtopic, please try to avoid these super long lines in mail :).

Ah. This is from the time when I posted patches via thunderbird and
disabled wrapping :) Is wrapping at 75 chars ok?


>> 
>> 
>>>>>>>> bool msi_enabled(const PCIDevice *dev) { return
>>>>>>>> msi_present(dev) && diff --git a/hw/msi.h b/hw/msi.h index
>>>>>>>> 75747ab..6ec1f99 100644 --- a/hw/msi.h +++ b/hw/msi.h @@
>>>>>>>> -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); 
>>>>>>>> diff --git a/hw/msix.c b/hw/msix.c index ded3c55..5f7d6d3
>>>>>>>> 100644 --- a/hw/msix.c +++ b/hw/msix.c @@ -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. diff --git a/hw/msix.h
>>>>>>>> b/hw/msix.h index 50aee82..26a437e 100644 --- a/hw/msix.h 
>>>>>>>> +++ b/hw/msix.h @@ -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); -- 1.7.10
>>>>>>>> 
>>>>>>>> ps. double '-' and git version is an end-of-patch scissor
>>>>>>>> as I read somewhere, cannot recall where exactly :)
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 21/06/12 20:56, Jan Kiszka wrote:
>>>>>>>>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
>>>>>>>>>> On 21/06/12 20:38, Jan Kiszka wrote:
>>>>>>>>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
>>>>>>>>>>>> On 21/06/12 17:39, Jan Kiszka wrote:
>>>>>>>>>>>>> On 2012-06-21 09:18, Alexey Kardashevskiy
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>> <aik@ozlabs.ru> --- hw/msi.c  |   29
>>>>>>>>>>>>>> +++++++++++++++++++++++++++++ hw/msi.h  |    2
>>>>>>>>>>>>>> ++ hw/msix.c |   11 ++++++++++- hw/msix.h |
>>>>>>>>>>>>>> 3 +++ 4 files changed, 44 insertions(+), 1
>>>>>>>>>>>>>> deletion(-)
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> diff --git a/hw/msi.c b/hw/msi.c index
>>>>>>>>>>>>>> 5233204..9ad84a4 100644 --- a/hw/msi.c +++
>>>>>>>>>>>>>> b/hw/msi.c @@ -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); }
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
>>>>>>>>>>>>> 
>>>>>>>>>>>>> MSIMessage msi_get_message(PCIDevice *dev,
>>>>>>>>>>>>> unsigned vector)
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> Who/how/why is going to calculate the vector
>>>>>>>>>>>> here?
>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>>>> +{ +    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.
>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> 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.
>>>>>>>>>> 
>>>>>>>>>> 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.
>>>>>>>>> 
>>>>>>>>> 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.
>> 
>> 
>> -- Alexey
>> 


-- 
Alexey

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Michael S. Tsirkin]

On Fri, Jul 20, 2012 at 12:50:26AM +1000, Alexey Kardashevskiy wrote:
> On 20/07/12 00:43, Michael S. Tsirkin wrote:
> > On Fri, Jul 20, 2012 at 12:24:05AM +1000, Alexey Kardashevskiy wrote:
> >> One comment below.
> >> 
> >> 
> >> On 19/07/12 19:27, Michael S. Tsirkin wrote:
> >>> On Thu, Jul 19, 2012 at 10:32:40AM +1000, Alexey Kardashevskiy
> >>> wrote:
> >>>> On 19/07/12 01:23, Michael S. Tsirkin wrote:
> >>>>> On Wed, Jul 18, 2012 at 11:17:12PM +1000, Alexey Kardashevskiy
> >>>>> wrote:
> >>>>>> On 18/07/12 22:43, Michael S. Tsirkin wrote:
> >>>>>>> On Thu, Jun 21, 2012 at 09:39:10PM +1000, Alexey
> >>>>>>> Kardashevskiy wrote:
> >>>>>>>> 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 <aik@ozlabs.ru>
> >>>>>>> 
> >>>>>>> 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.
> >>>>> 
> >>>>> 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.
> >>> 
> >>>>>>> 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.
> >>>>> 
> >>>>>>> Can't we just fix guest drivers to program the vectors
> >>>>>>> properly?
> >>>>>>> 
> >>>>>>> Also pls address the comment below.
> >>>>>> 
> >>>>>> Comment below.
> >>>>>> 
> >>>>>>> Thanks!
> >>>>>>> 
> >>>>>>>> --- hw/msi.c  |   13 +++++++++++++ hw/msi.h  |    1 + 
> >>>>>>>> hw/msix.c |    9 +++++++++ hw/msix.h |    2 ++ 4 files
> >>>>>>>> changed, 25 insertions(+)
> >>>>>>>> 
> >>>>>>>> diff --git a/hw/msi.c b/hw/msi.c index 5233204..cc6102f
> >>>>>>>> 100644 --- a/hw/msi.c +++ b/hw/msi.c @@ -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); +} +
> >>>>>>> 
> >>>>>>> Please add documentation. Something like
> >>>>>>> 
> >>>>>>> /* * 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.
> >>>> 
> >>>> 
> >>>> 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 :)
> >> 
> >> 
> >> It depends on how to look at it. Enabling MSI via the config space is
> >> also done via a special set of hypervisor calls (common and
> >> IBM-specific) so it is all hidden in one place - the system firmware,
> >> what is cool - no PHB drivers in the guest. Although MSI would not
> >> need any additional hypercall to init vectors (everything can be done
> >> via config space), there is MSI-X which stores vectors in BAR and
> >> there is no hypercall for BARs as they are simply memory mapped. This
> >> is I think why the firmware people (or phyp but it is probably the
> >> same) added IBM-specific MSI/MSIX config hypercalls.
> > 
> > Well what's wrong with guest doing this through a memory mapped
> > interface?
> 
> 
> Should not guest allocate addresses and program PHB with them?

What are you asking about? How does MSIX work normally?
OS gets the vector (address/data) pairs in some arch specific way
and then programs them into devices. No need for firmware to touch
any devices.

> The idea was to hide PHB details in the system firmware, this is the point.

The actual result is POWER behaves differently from almost any other
architecture.

> >> And I do not quite understand why MSIX people could not use extended
> >> PCI config space which is 4096 bytes, quite a lot, enough to fit 256
> >> vectors (have not seen a card which asked for more than 9 _per
> >> function_). If somebody really needs 2048, he may want 16384 as well
> >> (or any other crazy number), etc, so why did they put such a limit, it
> >> is a BAR, it is huge? :) A, offtopic anyway.
> 
> 
> > Well you have just described MSI, just don't use MSIX.
> > 
> > The motivation for MSIX was as follows: PCI/PCI-X config space is not
> > 4096 bytes, it is 256 bytes, and is very crowded. You are thinking of
> > PCI express. 
> 
> MSIX is PCIe feature, no?

No.

> > Config accesses are also nonposted which means at most one
> > must be in flight. This is not appropriate for vector programming which
> > needs to be done from multiple CPUs in parallel.
> 
> > Also offtopic, please try to avoid these super long lines in mail :).
> 
> Ah. This is from the time when I posted patches via thunderbird and
> disabled wrapping :) Is wrapping at 75 chars ok?

Sure.

> 
> >> 
> >> 
> >>>>>>>> bool msi_enabled(const PCIDevice *dev) { return
> >>>>>>>> msi_present(dev) && diff --git a/hw/msi.h b/hw/msi.h index
> >>>>>>>> 75747ab..6ec1f99 100644 --- a/hw/msi.h +++ b/hw/msi.h @@
> >>>>>>>> -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); 
> >>>>>>>> diff --git a/hw/msix.c b/hw/msix.c index ded3c55..5f7d6d3
> >>>>>>>> 100644 --- a/hw/msix.c +++ b/hw/msix.c @@ -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. diff --git a/hw/msix.h
> >>>>>>>> b/hw/msix.h index 50aee82..26a437e 100644 --- a/hw/msix.h 
> >>>>>>>> +++ b/hw/msix.h @@ -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); -- 1.7.10
> >>>>>>>> 
> >>>>>>>> ps. double '-' and git version is an end-of-patch scissor
> >>>>>>>> as I read somewhere, cannot recall where exactly :)
> >>>>>>>> 
> >>>>>>>> 
> >>>>>>>> 
> >>>>>>>> 
> >>>>>>>> 
> >>>>>>>> 
> >>>>>>>> On 21/06/12 20:56, Jan Kiszka wrote:
> >>>>>>>>> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
> >>>>>>>>>> On 21/06/12 20:38, Jan Kiszka wrote:
> >>>>>>>>>>> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
> >>>>>>>>>>>> On 21/06/12 17:39, Jan Kiszka wrote:
> >>>>>>>>>>>>> On 2012-06-21 09:18, Alexey Kardashevskiy
> >>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>> 
> >>>>>>>>>>>>>> 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
> >>>>>>>>>>>>>> <aik@ozlabs.ru> --- hw/msi.c  |   29
> >>>>>>>>>>>>>> +++++++++++++++++++++++++++++ hw/msi.h  |    2
> >>>>>>>>>>>>>> ++ hw/msix.c |   11 ++++++++++- hw/msix.h |
> >>>>>>>>>>>>>> 3 +++ 4 files changed, 44 insertions(+), 1
> >>>>>>>>>>>>>> deletion(-)
> >>>>>>>>>>>>>> 
> >>>>>>>>>>>>>> diff --git a/hw/msi.c b/hw/msi.c index
> >>>>>>>>>>>>>> 5233204..9ad84a4 100644 --- a/hw/msi.c +++
> >>>>>>>>>>>>>> b/hw/msi.c @@ -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); }
> >>>>>>>>>>>>>> 
> >>>>>>>>>>>>>> +MSIMessage msi_get_message(PCIDevice *dev)
> >>>>>>>>>>>>> 
> >>>>>>>>>>>>> MSIMessage msi_get_message(PCIDevice *dev,
> >>>>>>>>>>>>> unsigned vector)
> >>>>>>>>>>>> 
> >>>>>>>>>>>> 
> >>>>>>>>>>>> Who/how/why is going to calculate the vector
> >>>>>>>>>>>> here?
> >>>>>>>>>>>> 
> >>>>>>>>>>>>> 
> >>>>>>>>>>>>>> +{ +    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.
> >>>>>>>>>>>> 
> >>>>>>>>>>>> 
> >>>>>>>>>>>> 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.
> >>>>>>>>>> 
> >>>>>>>>>> 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.
> >>>>>>>>> 
> >>>>>>>>> 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.
> >> 
> >> 
> >> -- Alexey
> >> 
> 
> 
> -- 
> Alexey
> 

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

[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 <aik@ozlabs.ru>
---
 hw/msi.c  |   17 +++++++++++++++++
 hw/msi.h  |    1 +
 hw/msix.c |   13 +++++++++++++
 hw/msix.h |    1 +
 4 files changed, 32 insertions(+)

diff --git a/hw/msi.c b/hw/msi.c
index 5233204..e2273a0 100644
--- a/hw/msi.c
+++ b/hw/msi.c
@@ -105,6 +105,23 @@ 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);
 }
 
+/*
+ * Special API for POWER to configure the vectors through
+ * a side channel. Should never be used by devices.
+ */
+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) &&
diff --git a/hw/msi.h b/hw/msi.h
index 75747ab..6ec1f99 100644
--- a/hw/msi.h
+++ b/hw/msi.h
@@ -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);
diff --git a/hw/msix.c b/hw/msix.c
index fd9ea95..800fc32 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -37,6 +37,19 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
     return msg;
 }
 
+/*
+ * Special API for POWER to configure the vectors through
+ * a side channel. Should never be used by devices.
+ */
+void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
+{
+    uint8_t *table_entry = dev->msix_table + 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;
+}
+
 static uint8_t msix_pending_mask(int vector)
 {
     return 1 << (vector % 8);
diff --git a/hw/msix.h b/hw/msix.h
index 1786e27..15211cb 100644
--- a/hw/msix.h
+++ b/hw/msix.h
@@ -4,6 +4,7 @@
 #include "qemu-common.h"
 #include "pci.h"
 
+void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
 int msix_init(PCIDevice *dev, unsigned short nentries,
               MemoryRegion *table_bar, uint8_t table_bar_nr,
               unsigned table_offset, MemoryRegion *pba_bar,
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] msi/msix: added API to set MSI message address and data

[Michael S. Tsirkin]

On Thu, Jul 19, 2012 at 10:35:07AM +1000, Alexey Kardashevskiy wrote:
> 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 <aik@ozlabs.ru>

Thanks, applied.

> ---
>  hw/msi.c  |   17 +++++++++++++++++
>  hw/msi.h  |    1 +
>  hw/msix.c |   13 +++++++++++++
>  hw/msix.h |    1 +
>  4 files changed, 32 insertions(+)
> 
> diff --git a/hw/msi.c b/hw/msi.c
> index 5233204..e2273a0 100644
> --- a/hw/msi.c
> +++ b/hw/msi.c
> @@ -105,6 +105,23 @@ 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);
>  }
>  
> +/*
> + * Special API for POWER to configure the vectors through
> + * a side channel. Should never be used by devices.
> + */
> +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) &&
> diff --git a/hw/msi.h b/hw/msi.h
> index 75747ab..6ec1f99 100644
> --- a/hw/msi.h
> +++ b/hw/msi.h
> @@ -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);
> diff --git a/hw/msix.c b/hw/msix.c
> index fd9ea95..800fc32 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -37,6 +37,19 @@ static MSIMessage msix_get_message(PCIDevice *dev, unsigned vector)
>      return msg;
>  }
>  
> +/*
> + * Special API for POWER to configure the vectors through
> + * a side channel. Should never be used by devices.
> + */
> +void msix_set_message(PCIDevice *dev, int vector, struct MSIMessage msg)
> +{
> +    uint8_t *table_entry = dev->msix_table + 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;
> +}
> +
>  static uint8_t msix_pending_mask(int vector)
>  {
>      return 1 << (vector % 8);
> diff --git a/hw/msix.h b/hw/msix.h
> index 1786e27..15211cb 100644
> --- a/hw/msix.h
> +++ b/hw/msix.h
> @@ -4,6 +4,7 @@
>  #include "qemu-common.h"
>  #include "pci.h"
>  
> +void msix_set_message(PCIDevice *dev, int vector, MSIMessage msg);
>  int msix_init(PCIDevice *dev, unsigned short nentries,
>                MemoryRegion *table_bar, uint8_t table_bar_nr,
>                unsigned table_offset, MemoryRegion *pba_bar,
> -- 
> 1.7.10.4

Re: [Qemu-devel] [PATCH] msi/msix: added public API to set/get MSI message address, and data

[Alex Williamson]

On Thu, 2012-06-21 at 12:56 +0200, Jan Kiszka wrote:
> On 2012-06-21 12:50, Alexey Kardashevskiy wrote:
> > On 21/06/12 20:38, Jan Kiszka wrote:
> >> On 2012-06-21 12:28, Alexey Kardashevskiy wrote:
> >>> On 21/06/12 17:39, Jan Kiszka wrote:
> >>>> On 2012-06-21 09:18, Alexey Kardashevskiy wrote:
> >>>>>
> >>>>> 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 <aik@ozlabs.ru>
> >>>>> ---
> >>>>>  hw/msi.c  |   29 +++++++++++++++++++++++++++++
> >>>>>  hw/msi.h  |    2 ++
> >>>>>  hw/msix.c |   11 ++++++++++-
> >>>>>  hw/msix.h |    3 +++
> >>>>>  4 files changed, 44 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/hw/msi.c b/hw/msi.c
> >>>>> index 5233204..9ad84a4 100644
> >>>>> --- a/hw/msi.c
> >>>>> +++ b/hw/msi.c
> >>>>> @@ -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);
> >>>>>  }
> >>>>>  
> >>>>> +MSIMessage msi_get_message(PCIDevice *dev)
> >>>>
> >>>> MSIMessage msi_get_message(PCIDevice *dev, unsigned vector)
> >>>
> >>>
> >>> Who/how/why is going to calculate the vector here?
> >>>
> >>>>
> >>>>> +{
> >>>>> +    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.
> >>>
> >>>
> >>> 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.
> > 
> > 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.
> 
> 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.

Curse my timezone for not getting to respond before Alexey dropped the
'get' function.  msi_get_message(PCIDevice *dev, int vector) is
necessary for vfio so that we can make use of irqfds in KVM.  I've
actually got this function coded into vfio for the moment because it's
so much fun to get pci changes into qemu.  When MSI gets enabled, we
setup an eventfd in vfio for each vector (linux only supports a single
MSI vector, but we pretend we can support the fully specification).  If
we can enabled irqfd, we walk through each vector, getting an MSIMessage
to program into KVM, which sets the MSI to be injected when the eventfd
fires.  MSI is non-symmetric because the vectors have to be consecutive.
So it doesn't make sense to have a 'set' with a vector number, but a
'get' with the vector number is useful.  Perhaps it should be
msi_set_message(PCIDevice *dev, MSIMessage *msg, int nr_vectors), but I
don't know how POWER is using it either.  Thanks,

Alex

[Qemu-devel] [PATCH 2/3] pseries: added allocator for a block of IRQs

[Alexey Kardashevskiy]

The patch adds a simple helper which allocates a consecutive sequence
of IRQs calling spapr_allocate_irq for each and checks that allocated
IRQs go consequently.

The patch is required for upcoming support of MSI/MSIX on POWER.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/spapr.c |   19 +++++++++++++++++++
 hw/spapr.h |    1 +
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/hw/spapr.c b/hw/spapr.c
index 2e0b4b8..ef6ffcb 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -113,6 +113,25 @@ qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num,
     return qirq;
 }

+/* Allocate block of consequtive IRQs, returns a number of the first */
+int spapr_allocate_irq_block(uint32_t num, enum xics_irq_type type)
+{
+    int i, ret;
+    uint32_t irq = -1;
+
+    for (i = 0; i < num; ++i) {
+        if (!spapr_allocate_irq(0, &irq, type)) {
+            return -1;
+        }
+        if (0 == i) {
+            ret = irq;
+        } else if (ret + i != irq) {
+            return -1;
+        }
+    }
+    return ret;
+}
+
 static int spapr_set_associativity(void *fdt, sPAPREnvironment *spapr)
 {
     int ret = 0, offset;
diff --git a/hw/spapr.h b/hw/spapr.h
index 502393a..408b470 100644
--- a/hw/spapr.h
+++ b/hw/spapr.h
@@ -289,6 +289,7 @@ target_ulong spapr_hypercall(CPUPPCState *env, target_ulong opcode,

 qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num,
                             enum xics_irq_type type);
+int spapr_allocate_irq_block(uint32_t num, enum xics_irq_type type);

 static inline qemu_irq spapr_allocate_msi(uint32_t hint, uint32_t *irq_num)
 {
-- 
1.7.7.3

[Qemu-devel] [PATCH 3/3] pseries pci: added MSI/MSIX support

[Alexey Kardashevskiy]

virtio-pci expects the guest to set up MSI message address and data, and
to do other initialization such as a vector number negotiation.
It also notifies the guest via writing an MSI message to a previously set
address.

This patch includes:

1. RTAS call "ibm,change-msi" which sets up number of MSI vectors per
a device. Note that this call may configure and return lesser number of
vectors than requested.

2. RTAS call "ibm,query-interrupt-source-number" which translates MSI
vector to interrupt controller (XICS) IRQ number.

3. A "config_space_address to msi_table" map to provide IRQ resolving from
config-address as MSI RTAS calls take a PCI config space address as
an identifier.

4. A MSIX memory region is added to catch msi_notify()/msix_notiry()
from virtio-pci and pass them to the guest via qemu_irq_pulse().

This patch depends on the "msi/msix: added functions to API to set up
message address and data" patch.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/spapr.c     |    9 ++-
 hw/spapr_pci.c |  266 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/spapr_pci.h |   13 +++-
 trace-events   |    9 ++
 4 files changed, 284 insertions(+), 13 deletions(-)

diff --git a/hw/spapr.c b/hw/spapr.c
index ef6ffcb..35ad075 100644
--- a/hw/spapr.c
+++ b/hw/spapr.c
@@ -43,6 +43,7 @@
 #include "hw/spapr_vio.h"
 #include "hw/spapr_pci.h"
 #include "hw/xics.h"
+#include "hw/msi.h"

 #include "kvm.h"
 #include "kvm_ppc.h"
@@ -82,6 +83,7 @@
 #define SPAPR_PCI_MEM_WIN_ADDR  (0x10000000000ULL + 0xA0000000)
 #define SPAPR_PCI_MEM_WIN_SIZE  0x20000000
 #define SPAPR_PCI_IO_WIN_ADDR   (0x10000000000ULL + 0x80000000)
+#define SPAPR_PCI_MSI_WIN_ADDR  (0x10000000000ULL + 0x90000000)

 #define PHANDLE_XICP            0x00001111

@@ -116,7 +118,7 @@ qemu_irq spapr_allocate_irq(uint32_t hint, uint32_t *irq_num,
 /* Allocate block of consequtive IRQs, returns a number of the first */
 int spapr_allocate_irq_block(uint32_t num, enum xics_irq_type type)
 {
-    int i, ret;
+    int i, ret = 0;
     uint32_t irq = -1;

     for (i = 0; i < num; ++i) {
@@ -690,6 +692,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
     long pteg_shift = 17;
     char *filename;

+    msi_supported = true;
+
     spapr = g_malloc0(sizeof(*spapr));
     QLIST_INIT(&spapr->phbs);

@@ -804,7 +808,8 @@ static void ppc_spapr_init(ram_addr_t ram_size,
     spapr_create_phb(spapr, "pci", SPAPR_PCI_BUID,
                      SPAPR_PCI_MEM_WIN_ADDR,
                      SPAPR_PCI_MEM_WIN_SIZE,
-                     SPAPR_PCI_IO_WIN_ADDR);
+                     SPAPR_PCI_IO_WIN_ADDR,
+                     SPAPR_PCI_MSI_WIN_ADDR);

     for (i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index 93017cd..21fbc50 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -24,31 +24,46 @@
  */
 #include "hw.h"
 #include "pci.h"
+#include "msix.h"
+#include "msi.h"
 #include "pci_host.h"
 #include "hw/spapr.h"
 #include "hw/spapr_pci.h"
 #include "exec-memory.h"
 #include <libfdt.h>
+#include "trace.h"

 #include "hw/pci_internals.h"

-static PCIDevice *find_dev(sPAPREnvironment *spapr,
-                           uint64_t buid, uint32_t config_addr)
+static sPAPRPHBState *find_phb(sPAPREnvironment *spapr, uint64_t buid)
 {
-    DeviceState *qdev;
-    int devfn = (config_addr >> 8) & 0xFF;
     sPAPRPHBState *phb;

     QLIST_FOREACH(phb, &spapr->phbs, list) {
         if (phb->buid != buid) {
             continue;
         }
+        return phb;
+    }

-        QTAILQ_FOREACH(qdev, &phb->host_state.bus->qbus.children, sibling) {
-            PCIDevice *dev = (PCIDevice *)qdev;
-            if (dev->devfn == devfn) {
-                return dev;
-            }
+    return NULL;
+}
+
+static PCIDevice *find_dev(sPAPREnvironment *spapr, uint64_t buid,
+                           uint32_t config_addr)
+{
+    sPAPRPHBState *phb = find_phb(spapr, buid);
+    DeviceState *qdev;
+    int devfn = (config_addr >> 8) & 0xFF;
+
+    if (!phb) {
+        return NULL;
+    }
+
+    QTAILQ_FOREACH(qdev, &phb->host_state.bus->qbus.children, sibling) {
+        PCIDevice *dev = (PCIDevice *)qdev;
+        if (dev->devfn == devfn) {
+            return dev;
         }
     }

@@ -138,6 +153,220 @@ static void rtas_write_pci_config(sPAPREnvironment *spapr,
     rtas_st(rets, 0, 0);
 }

+/*
+ * Initializes req_num vectors for a device.
+ * The code assumes that MSI/MSIX is enabled in the config space
+ * as a result of msix_init() or msi_init().
+ */
+static int spapr_pci_config_msi(sPAPRPHBState *ph, int ndev,
+                                PCIDevice *pdev, bool msix, unsigned req_num)
+{
+    unsigned i;
+    int irq;
+    uint64_t msi_address;
+    uint32_t config_addr = pdev->devfn << 8;
+
+    /* Disabling - nothing to do */
+    if (0 == req_num) {
+        return 0;
+    }
+
+    /* Enabling! */
+    if (ph->msi_table[ndev].nvec && (req_num != ph->msi_table[ndev].nvec)) {
+        /* Unexpected behaviour */
+        fprintf(stderr, "Cannot reuse cached MSI config for device#%u", ndev);
+        return -1;
+    }
+
+    if (0 == ph->msi_table[ndev].nvec) {
+        irq = spapr_allocate_irq_block(req_num, XICS_MSI);
+        if (irq < 0) {
+            return -1;
+        }
+        ph->msi_table[ndev].dt_irq = irq;
+        ph->msi_table[ndev].nvec = req_num;
+        ph->msi_table[ndev].config_addr = config_addr;
+    }
+
+    if (msix) {
+        for (i = 0; i < req_num; ++i) {
+            msi_address = ph->msi_win_addr + ((ndev << 16) | i << 2);
+            msix_set_address_data(pdev, i, msi_address, 0);
+            trace_spapr_pci_msi_setup(pdev->name, i, msi_address);
+        }
+    } else {
+        msi_address = ph->msi_win_addr + (ndev << 16);
+        msi_set_address_data(pdev, msi_address, 0);
+        trace_spapr_pci_msi_setup(pdev->name, 0, msi_address);
+    }
+
+    return req_num;
+}
+
+/*
+ * The handler handles both MSI and MSIX.
+ * For MSI-X, the vector number is encoded as a part of the address
+ * and data is set to 0.
+ * For MSI, the vector takes least bits in data and not encoded in
+ * address.
+ */
+static void spapr_msi_write(void *opaque, target_phys_addr_t addr,
+                            uint64_t data, unsigned size)
+{
+    sPAPRPHBState *ephb = opaque;
+    int n = addr >> 16;
+    int vec = ((addr & 0xFFFF) >> 2) | data;
+    uint32_t dt_irq = ephb->msi_table[n].dt_irq + vec;
+
+    trace_spapr_pci_msi_write(addr, data, dt_irq);
+
+    qemu_irq_pulse(xics_assign_irq(spapr->icp, dt_irq, XICS_MSI));
+}
+
+static const MemoryRegionOps spapr_msi_ops = {
+    .read = NULL,
+    .write = spapr_msi_write,
+    .endianness = DEVICE_LITTLE_ENDIAN
+};
+
+static void rtas_ibm_change_msi(sPAPREnvironment *spapr,
+                                uint32_t token, uint32_t nargs,
+                                target_ulong args, uint32_t nret,
+                                target_ulong rets)
+{
+    uint32_t config_addr = rtas_ld(args, 0);
+    uint64_t buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+#define RTAS_QUERY_FN           0
+#define RTAS_CHANGE_FN          1
+#define RTAS_RESET_FN           2
+#define RTAS_CHANGE_MSI_FN      3
+#define RTAS_CHANGE_MSIX_FN     4
+    unsigned int func = rtas_ld(args, 3);
+    unsigned int req_num = rtas_ld(args, 4); /* 0 == remove all */
+    unsigned int seq_num = rtas_ld(args, 5);
+    int assigned_num = -1;
+    enum { MSI = 1, MSIX = 2 } ret_intr_type = MSI;
+    int i;
+    sPAPRPHBState *phb = NULL;
+    PCIDevice *pdev = NULL;
+
+    switch (func) {
+    case RTAS_CHANGE_MSI_FN:
+    case RTAS_CHANGE_FN:
+        ret_intr_type = MSI;
+        break;
+    case RTAS_CHANGE_MSIX_FN:
+        ret_intr_type = MSIX;
+        break;
+    default:
+        fprintf(stderr, "rtas_ibm_change_msi(%u) is not implemented\n", func);
+        rtas_st(rets, 0, -3); /* Parameter error */
+        return;
+    }
+
+    /* Fins sPAPRPHBState */
+    phb = find_phb(spapr, buid);
+    if (phb) {
+        pdev = find_dev(spapr, buid, config_addr);
+    }
+    if (!phb || !pdev) {
+        rtas_st(rets, 0, -3); /* Parameter error */
+        return;
+    }
+
+    if (0 == req_num) {
+        /* Calculate a device number in the map to remove */
+        for (i = 0; i < SPAPR_MSIX_MAX_DEVS; ++i) {
+            if (phb->msi_table[i].nvec &&
+                    (phb->msi_table[i].config_addr == config_addr)) {
+                trace_spapr_pci_msi("Releasing MSI config for device#", i);
+                break;
+            }
+        }
+        if (i >= SPAPR_MSIX_MAX_DEVS) {
+            fprintf(stderr, "MSI has not been enabled for this device!\n");
+            rtas_st(rets, 0, -1); /* Hardware error */
+            return;
+        }
+    } else {
+        /* Find a device number in the map to add */
+        for (i = 0; i < SPAPR_MSIX_MAX_DEVS; ++i) {
+            if (!phb->msi_table[i].nvec) {
+                /* as we never free entries, it means no more left in cache,
+                   so we use new empty entry */
+                trace_spapr_pci_msi("Allocating new MSI for device#", i);
+                break;
+            }
+            if (phb->msi_table[i].config_addr != config_addr) {
+                continue;
+            }
+            /* Even if req_num is greater than nvec, it is expected
+               that the host will allocate (nvec) vectors again */
+            trace_spapr_pci_msi("Reuse cached MSI config for device#", i);
+            break;
+        }
+        if (i >= SPAPR_MSIX_MAX_DEVS) {
+            fprintf(stderr, "No free entry for a new MSI device\n");
+            rtas_st(rets, 0, -1); /* Hardware error */
+            return;
+        }
+    }
+
+    assigned_num = spapr_pci_config_msi(phb, i, pdev,
+                                        ret_intr_type == MSIX, req_num);
+
+    if (assigned_num < 0) {
+        fprintf(stderr, "rtas_ibm_change_msi(%u) failed\n", func);
+        rtas_st(rets, 0, -3); /* Parameter error */
+        return;
+    }
+    trace_spapr_pci_rtas_ibm_change_msi(func, req_num, assigned_num);
+
+    rtas_st(rets, 0, 0);
+    rtas_st(rets, 1, assigned_num);
+    rtas_st(rets, 2, ++seq_num);
+    rtas_st(rets, 3, ret_intr_type);
+}
+
+static void rtas_ibm_query_interrupt_source_number(sPAPREnvironment *spapr,
+                                                   uint32_t token,
+                                                   uint32_t nargs,
+                                                   target_ulong args,
+                                                   uint32_t nret,
+                                                   target_ulong rets)
+{
+    uint32_t config_addr = rtas_ld(args, 0);
+    uint64_t buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    unsigned int i, intr_src_num = -1, ioa_intr_num = rtas_ld(args, 3);
+    sPAPRPHBState *phb = NULL;
+
+    /* Fins sPAPRPHBState */
+    phb = find_phb(spapr, buid);
+    if (!phb) {
+        rtas_st(rets, 0, -3); /* Parameter error */
+        return;
+    }
+
+    /* Find device descriptor and start IRQ */
+    for (i = 0; i < SPAPR_MSIX_MAX_DEVS; ++i) {
+        if (phb->msi_table[i].config_addr == config_addr) {
+            intr_src_num = phb->msi_table[i].dt_irq + ioa_intr_num;
+            break;
+        }
+    }
+    /* Device has not been configured, return error */
+    if (-1 == intr_src_num) {
+        rtas_st(rets, 0, -1); /* Hardware error */
+        return;
+    }
+    trace_spapr_pci_rtas_ibm_query_interrupt_source_number(ioa_intr_num,
+                                                           intr_src_num);
+
+    rtas_st(rets, 0, 0);
+    rtas_st(rets, 1, intr_src_num);
+    rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
+}
+
 static int pci_swizzle(int slot, int pin)
 {
     return (slot + pin) % PCI_NUM_PINS;
@@ -162,6 +391,8 @@ static void pci_spapr_set_irq(void *opaque, int irq_num, int level)
      */
     sPAPRPHBState *phb = opaque;

+    trace_spapr_pci_lsi_set(phb->busname, irq_num,
+                            phb->lsi_table[irq_num].dt_irq);
     qemu_set_irq(xics_assign_irq(spapr->icp,
                                  phb->lsi_table[irq_num].dt_irq, XICS_LSI),
                  level);
@@ -255,6 +486,14 @@ static int spapr_phb_init(SysBusDevice *s)
     memory_region_add_subregion(get_system_memory(), phb->io_win_addr,
                                 &phb->iowindow);

+    if (msi_supported) {
+        sprintf(namebuf, "%s.msi", phb->dtbusname);
+        memory_region_init_io(&phb->msiwindow, &spapr_msi_ops, phb,
+                              namebuf, SPAPR_MSIX_MAX_DEVS * 0x10000);
+        memory_region_add_subregion(get_system_memory(), phb->msi_win_addr,
+                                    &phb->msiwindow);
+    }
+
     bus = pci_register_bus(&phb->host_state.busdev.qdev,
                            phb->busname ? phb->busname : phb->dtbusname,
                            pci_spapr_set_irq, pci_spapr_map_irq, phb,
@@ -286,6 +525,7 @@ static Property spapr_phb_properties[] = {
     DEFINE_PROP_HEX64("mem_win_size", sPAPRPHBState, mem_win_size, 0x20000000),
     DEFINE_PROP_HEX64("io_win_addr", sPAPRPHBState, io_win_addr, 0),
     DEFINE_PROP_HEX64("io_win_size", sPAPRPHBState, io_win_size, 0x10000),
+    DEFINE_PROP_HEX64("msi_win_addr", sPAPRPHBState, msi_win_addr, 0),
     DEFINE_PROP_END_OF_LIST(),
 };

@@ -301,6 +541,11 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     spapr_rtas_register("write-pci-config", rtas_write_pci_config);
     spapr_rtas_register("ibm,read-pci-config", rtas_ibm_read_pci_config);
     spapr_rtas_register("ibm,write-pci-config", rtas_ibm_write_pci_config);
+    if (msi_supported) {
+        spapr_rtas_register("ibm,query-interrupt-source-number",
+                            rtas_ibm_query_interrupt_source_number);
+        spapr_rtas_register("ibm,change-msi", rtas_ibm_change_msi);
+    }
 }

 static TypeInfo spapr_phb_info = {
@@ -313,7 +558,7 @@ static TypeInfo spapr_phb_info = {
 void spapr_create_phb(sPAPREnvironment *spapr,
                       const char *busname, uint64_t buid,
                       uint64_t mem_win_addr, uint64_t mem_win_size,
-                      uint64_t io_win_addr)
+                      uint64_t io_win_addr, uint64_t msi_win_addr)
 {
     DeviceState *dev;

@@ -326,6 +571,7 @@ void spapr_create_phb(sPAPREnvironment *spapr,
     qdev_prop_set_uint64(dev, "mem_win_addr", mem_win_addr);
     qdev_prop_set_uint64(dev, "mem_win_size", mem_win_size);
     qdev_prop_set_uint64(dev, "io_win_addr", io_win_addr);
+    qdev_prop_set_uint64(dev, "msi_win_addr", msi_win_addr);

     qdev_init_nofail(dev);
 }
diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
index 11c3ee1..bb9a5ea 100644
--- a/hw/spapr_pci.h
+++ b/hw/spapr_pci.h
@@ -27,6 +27,8 @@
 #include "hw/pci_host.h"
 #include "hw/xics.h"

+#define SPAPR_MSIX_MAX_DEVS 32
+
 typedef struct sPAPRPHBState {
     PCIHostState host_state;

@@ -37,12 +39,21 @@ typedef struct sPAPRPHBState {
     MemoryRegion memspace, iospace;
     target_phys_addr_t mem_win_addr, mem_win_size, io_win_addr, io_win_size;
     MemoryRegion memwindow, iowindow;
+    target_phys_addr_t msi_win_addr;
+    MemoryRegion msiwindow;
+
     DMAContext *dma;

     struct {
         uint32_t dt_irq;
     } lsi_table[PCI_NUM_PINS];

+    struct {
+        uint32_t config_addr;
+        uint32_t dt_irq; /* first irq, dt_irq!=0 means "used" */
+        int nvec;
+    } msi_table[SPAPR_MSIX_MAX_DEVS];
+
     QLIST_ENTRY(sPAPRPHBState) list;
 } sPAPRPHBState;

@@ -52,7 +63,7 @@ typedef struct sPAPRPHBState {
 void spapr_create_phb(sPAPREnvironment *spapr,
                       const char *busname, uint64_t buid,
                       uint64_t mem_win_addr, uint64_t mem_win_size,
-                      uint64_t io_win_addr);
+                      uint64_t io_win_addr, uint64_t msi_win_addr);

 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
diff --git a/trace-events b/trace-events
index 70f059d..8d94053 100644
--- a/trace-events
+++ b/trace-events
@@ -788,3 +788,12 @@ qxl_render_blit_guest_primary_initialized(void) ""
 qxl_render_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom)
"stride=%d [%d, %d, %d, %d]"
 qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp,
int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d"
 qxl_render_update_area_done(void *cookie) "%p"
+
+# hw/spapr_pci.c
+spapr_pci_msi(const char *msg, uint32_t n) "%s%u"
+spapr_pci_msi_setup(const char *name, unsigned vector, uint64_t addr) "dev\"%s\" vector %u,
addr=%"PRIx64
+spapr_pci_rtas_ibm_change_msi(unsigned func, unsigned req, int assigned) "func %u, requested %u,
assigned %d"
+spapr_pci_rtas_ibm_query_interrupt_source_number(unsigned ioa, unsigned intr) "queries for #%u, IRQ%u"
+spapr_pci_msi_write(uint64_t addr, uint64_t data, uint32_t dt_irq) "@%"PRIx64"<=%"PRIx64" IRQ %u"
+spapr_pci_lsi_set(const char *busname, int pin, uint32_t irq) "%s PIN%d IRQ %u"
+
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH 0/3] adding MSI/MSIX for PCI on POWER

[Alexey Kardashevskiy]

Forgot to CC: someone :)

On 14/06/12 14:29, Alexey Kardashevskiy wrote:
> The following patches add MSIX support for PCI on POWER.
> The first aim is virtio-pci so it was tested. It will also support
> VFIO when it becomes available in public.
> 
> Alexey Kardashevskiy (3):
>   msi/msix: added functions to API to set up message address and data
>   pseries: added allocator for a block of IRQs
>   pseries pci: added MSI/MSIX support
> 
>  hw/msi.c       |   14 +++
>  hw/msi.h       |    1 +
>  hw/msix.c      |   10 ++
>  hw/msix.h      |    3 +
>  hw/spapr.c     |   26 +++++-
>  hw/spapr.h     |    1 +
>  hw/spapr_pci.c |  266 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/spapr_pci.h |   13 +++-
>  trace-events   |    9 ++
>  9 files changed, 331 insertions(+), 12 deletions(-)
> 


-- 
Alexey