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@xxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
