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 <address@hidden>
> 
> 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 <address@hidden>
>>>>>>>> ---
>>>>>>>>  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