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

Jan

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