| Message ID | 34c1e72dbc96d7a3f03f064093d101e2f4cb03b2.1303554218.git.jan.kiszka@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Apr 23, 2011 at 12:23:35PM +0200, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > This structure will be used for legacy MSI as well and will become part > of the KVM API. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> I'd like KVMMSIMessage better. struct kvm_msi_message would be even better but oh well. > --- > hw/msix.c | 10 +++++----- > hw/pci.h | 10 ++-------- > kvm.h | 7 +++++++ > 3 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/hw/msix.c b/hw/msix.c > index 8b68a6a..42870c5 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -51,7 +51,7 @@ int msix_supported; > static void kvm_msix_free(PCIDevice *dev) > { > int vector, changed = 0; > - struct kvm_msix_message *kmm; > + KVMMsiMessage *kmm; > > for (vector = 0; vector < dev->msix_entries_nr; ++vector) { > if (dev->msix_entry_used[vector]) { > @@ -66,7 +66,7 @@ static void kvm_msix_free(PCIDevice *dev) > } > > static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector, > - struct kvm_msix_message *kmm) > + KVMMsiMessage *kmm) > { > uint8_t *table_entry = dev->msix_table_page + vector * MSIX_ENTRY_SIZE; > > @@ -78,7 +78,7 @@ static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector, > static void kvm_msix_update(PCIDevice *dev, int vector, > int was_masked, int is_masked) > { > - struct kvm_msix_message e = {}, *entry; > + KVMMsiMessage e = {}, *entry; > int mask_cleared = was_masked && !is_masked; > /* It is only legal to change an entry when it is masked. Therefore, it is > * enough to update the routing in kernel when mask is being cleared. */ > @@ -114,7 +114,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector, > > static int kvm_msix_add(PCIDevice *dev, unsigned vector) > { > - struct kvm_msix_message *kmm = dev->msix_irq_entries + vector; > + KVMMsiMessage *kmm = dev->msix_irq_entries + vector; > int r; > > if (!kvm_has_gsi_routing()) { > @@ -147,7 +147,7 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector) > > static void kvm_msix_del(PCIDevice *dev, unsigned vector) > { > - struct kvm_msix_message *kmm; > + KVMMsiMessage *kmm; > > if (dev->msix_entry_used[vector]) { > return; > diff --git a/hw/pci.h b/hw/pci.h > index dd09494..dc5df17 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -5,6 +5,7 @@ > #include "qobject.h" > > #include "qdev.h" > +#include "kvm.h" I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h is anything wrong with that? Maybe just rename it to make it generic for msi and leave if where it is. > > /* PCI includes legacy ISA access. */ > #include "isa.h" > @@ -129,13 +130,6 @@ enum { > typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector, > int masked); > > -struct kvm_msix_message { > - uint32_t gsi; > - uint32_t addr_lo; > - uint32_t addr_hi; > - uint32_t data; > -}; > - > struct PCIDevice { > DeviceState qdev; > /* PCI config space */ > @@ -208,7 +202,7 @@ struct PCIDevice { > * on the rest of the region. */ > target_phys_addr_t msix_page_size; > > - struct kvm_msix_message *msix_irq_entries; > + KVMMsiMessage *msix_irq_entries; > > msix_mask_notifier_func msix_mask_notifier; > }; > diff --git a/kvm.h b/kvm.h > index b890b5d..8ece0b3 100644 > --- a/kvm.h > +++ b/kvm.h > @@ -214,6 +214,13 @@ int kvm_set_irqfd(int gsi, int fd, bool assigned) > > int kvm_set_ioeventfd_pio_word(int fd, uint16_t adr, uint16_t val, bool assign); > > +typedef struct KVMMsiMessage { > + uint32_t gsi; > + uint32_t addr_lo; > + uint32_t addr_hi; > + uint32_t data; > +} KVMMsiMessage; > + > int kvm_has_gsi_routing(void); > int kvm_get_irq_route_gsi(void); > int kvm_add_msix(uint32_t gsi, uint32_t addr_lo, > -- > 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2011-04-27 15:34, Michael S. Tsirkin wrote: > On Sat, Apr 23, 2011 at 12:23:35PM +0200, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> This structure will be used for legacy MSI as well and will become part >> of the KVM API. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > I'd like KVMMSIMessage better. struct kvm_msi_message would be even > better but oh well. Yeah, QEMU naming convention is not designed for such use cases... Will change it as you prefer. > >> --- >> hw/msix.c | 10 +++++----- >> hw/pci.h | 10 ++-------- >> kvm.h | 7 +++++++ >> 3 files changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/hw/msix.c b/hw/msix.c >> index 8b68a6a..42870c5 100644 >> --- a/hw/msix.c >> +++ b/hw/msix.c >> @@ -51,7 +51,7 @@ int msix_supported; >> static void kvm_msix_free(PCIDevice *dev) >> { >> int vector, changed = 0; >> - struct kvm_msix_message *kmm; >> + KVMMsiMessage *kmm; >> >> for (vector = 0; vector < dev->msix_entries_nr; ++vector) { >> if (dev->msix_entry_used[vector]) { >> @@ -66,7 +66,7 @@ static void kvm_msix_free(PCIDevice *dev) >> } >> >> static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector, >> - struct kvm_msix_message *kmm) >> + KVMMsiMessage *kmm) >> { >> uint8_t *table_entry = dev->msix_table_page + vector * MSIX_ENTRY_SIZE; >> >> @@ -78,7 +78,7 @@ static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector, >> static void kvm_msix_update(PCIDevice *dev, int vector, >> int was_masked, int is_masked) >> { >> - struct kvm_msix_message e = {}, *entry; >> + KVMMsiMessage e = {}, *entry; >> int mask_cleared = was_masked && !is_masked; >> /* It is only legal to change an entry when it is masked. Therefore, it is >> * enough to update the routing in kernel when mask is being cleared. */ >> @@ -114,7 +114,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector, >> >> static int kvm_msix_add(PCIDevice *dev, unsigned vector) >> { >> - struct kvm_msix_message *kmm = dev->msix_irq_entries + vector; >> + KVMMsiMessage *kmm = dev->msix_irq_entries + vector; >> int r; >> >> if (!kvm_has_gsi_routing()) { >> @@ -147,7 +147,7 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector) >> >> static void kvm_msix_del(PCIDevice *dev, unsigned vector) >> { >> - struct kvm_msix_message *kmm; >> + KVMMsiMessage *kmm; >> >> if (dev->msix_entry_used[vector]) { >> return; >> diff --git a/hw/pci.h b/hw/pci.h >> index dd09494..dc5df17 100644 >> --- a/hw/pci.h >> +++ b/hw/pci.h >> @@ -5,6 +5,7 @@ >> #include "qobject.h" >> >> #include "qdev.h" >> +#include "kvm.h" > > I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h > is anything wrong with that? Maybe just rename it to make it generic > for msi and leave if where it is. kvm.h shall provide kvm related types, not some unrelated header. That's even more important with MSI support for non-PCI devices (aka HPET). Jan
On 04/27/2011 05:04 PM, Jan Kiszka wrote: > > > > I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h > > is anything wrong with that? Maybe just rename it to make it generic > > for msi and leave if where it is. > > kvm.h shall provide kvm related types, not some unrelated header. That's > even more important with MSI support for non-PCI devices (aka HPET). We could have an MSIMessage type that abstracts the general facility, and let it embed a KVMMsiMessage that contains just the gsi.
On 2011-04-27 16:29, Avi Kivity wrote: > On 04/27/2011 05:04 PM, Jan Kiszka wrote: >>> >>> I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h >>> is anything wrong with that? Maybe just rename it to make it generic >>> for msi and leave if where it is. >> >> kvm.h shall provide kvm related types, not some unrelated header. That's >> even more important with MSI support for non-PCI devices (aka HPET). > > We could have an MSIMessage type that abstracts the general facility, > and let it embed a KVMMsiMessage that contains just the gsi. Yes, likely also useful for generic MSI delivery services that bypass stl_phys. Jan
On Wed, Apr 27, 2011 at 04:04:44PM +0200, Jan Kiszka wrote: > On 2011-04-27 15:34, Michael S. Tsirkin wrote: > > On Sat, Apr 23, 2011 at 12:23:35PM +0200, Jan Kiszka wrote: > >> From: Jan Kiszka <jan.kiszka@siemens.com> > >> > >> This structure will be used for legacy MSI as well and will become part > >> of the KVM API. > >> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > > > I'd like KVMMSIMessage better. struct kvm_msi_message would be even > > better but oh well. > > Yeah, QEMU naming convention is not designed for such use cases... Will > change it as you prefer. > > > > >> --- > >> hw/msix.c | 10 +++++----- > >> hw/pci.h | 10 ++-------- > >> kvm.h | 7 +++++++ > >> 3 files changed, 14 insertions(+), 13 deletions(-) > >> > >> diff --git a/hw/msix.c b/hw/msix.c > >> index 8b68a6a..42870c5 100644 > >> --- a/hw/msix.c > >> +++ b/hw/msix.c > >> @@ -51,7 +51,7 @@ int msix_supported; > >> static void kvm_msix_free(PCIDevice *dev) > >> { > >> int vector, changed = 0; > >> - struct kvm_msix_message *kmm; > >> + KVMMsiMessage *kmm; > >> > >> for (vector = 0; vector < dev->msix_entries_nr; ++vector) { > >> if (dev->msix_entry_used[vector]) { > >> @@ -66,7 +66,7 @@ static void kvm_msix_free(PCIDevice *dev) > >> } > >> > >> static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector, > >> - struct kvm_msix_message *kmm) > >> + KVMMsiMessage *kmm) > >> { > >> uint8_t *table_entry = dev->msix_table_page + vector * MSIX_ENTRY_SIZE; > >> > >> @@ -78,7 +78,7 @@ static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector, > >> static void kvm_msix_update(PCIDevice *dev, int vector, > >> int was_masked, int is_masked) > >> { > >> - struct kvm_msix_message e = {}, *entry; > >> + KVMMsiMessage e = {}, *entry; > >> int mask_cleared = was_masked && !is_masked; > >> /* It is only legal to change an entry when it is masked. Therefore, it is > >> * enough to update the routing in kernel when mask is being cleared. */ > >> @@ -114,7 +114,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector, > >> > >> static int kvm_msix_add(PCIDevice *dev, unsigned vector) > >> { > >> - struct kvm_msix_message *kmm = dev->msix_irq_entries + vector; > >> + KVMMsiMessage *kmm = dev->msix_irq_entries + vector; > >> int r; > >> > >> if (!kvm_has_gsi_routing()) { > >> @@ -147,7 +147,7 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector) > >> > >> static void kvm_msix_del(PCIDevice *dev, unsigned vector) > >> { > >> - struct kvm_msix_message *kmm; > >> + KVMMsiMessage *kmm; > >> > >> if (dev->msix_entry_used[vector]) { > >> return; > >> diff --git a/hw/pci.h b/hw/pci.h > >> index dd09494..dc5df17 100644 > >> --- a/hw/pci.h > >> +++ b/hw/pci.h > >> @@ -5,6 +5,7 @@ > >> #include "qobject.h" > >> > >> #include "qdev.h" > >> +#include "kvm.h" > > > > I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h > > is anything wrong with that? Maybe just rename it to make it generic > > for msi and leave if where it is. > > kvm.h shall provide kvm related types, not some unrelated header. That's > even more important with MSI support for non-PCI devices (aka HPET). > > Jan Generally qemu suggests forward-declaring types so all users don't need to pull them in. This should certainly work here: declare the type in qemu-common.h, then msix.c and kvm are the only ones that need to include kvm.h > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 27, 2011 at 05:29:10PM +0300, Avi Kivity wrote: > On 04/27/2011 05:04 PM, Jan Kiszka wrote: > >> > >> I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h > >> is anything wrong with that? Maybe just rename it to make it generic > >> for msi and leave if where it is. > > > >kvm.h shall provide kvm related types, not some unrelated header. That's > >even more important with MSI support for non-PCI devices (aka HPET). > > We could have an MSIMessage type that abstracts the general > facility, and let it embed a KVMMsiMessage that contains just the > gsi. The whole structure is only needed for kvm though: no one else seems to want an MSIMessage. > -- > error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/27/2011 05:30 PM, Jan Kiszka wrote: > On 2011-04-27 16:29, Avi Kivity wrote: > > On 04/27/2011 05:04 PM, Jan Kiszka wrote: > >>> > >>> I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h > >>> is anything wrong with that? Maybe just rename it to make it generic > >>> for msi and leave if where it is. > >> > >> kvm.h shall provide kvm related types, not some unrelated header. That's > >> even more important with MSI support for non-PCI devices (aka HPET). > > > > We could have an MSIMessage type that abstracts the general facility, > > and let it embed a KVMMsiMessage that contains just the gsi. > > Yes, likely also useful for generic MSI delivery services that bypass > stl_phys. > Right, so you can cache the phys_page and apic lookups. A different layer in which to accomplish this is to have a per-device tlb. So we'd have CachedPhysicalAddress msi_fsb_tlb; stl_phys_tlb(&msi_fsb_cache, addr, data); which caches the addr lookup in a 1-entry tlb. If we have many non-msi repeated device writes to the same address, this might be worthwhile. From a quick look at the code, I don't think we do, though.
On 2011-04-27 16:40, Avi Kivity wrote: > On 04/27/2011 05:30 PM, Jan Kiszka wrote: >> On 2011-04-27 16:29, Avi Kivity wrote: >>> On 04/27/2011 05:04 PM, Jan Kiszka wrote: >>>>> >>>>> I put kvm_msix_message in pci.h to avoid having every pci device pull in kvm.h >>>>> is anything wrong with that? Maybe just rename it to make it generic >>>>> for msi and leave if where it is. >>>> >>>> kvm.h shall provide kvm related types, not some unrelated header. That's >>>> even more important with MSI support for non-PCI devices (aka HPET). >>> >>> We could have an MSIMessage type that abstracts the general facility, >>> and let it embed a KVMMsiMessage that contains just the gsi. >> >> Yes, likely also useful for generic MSI delivery services that bypass >> stl_phys. >> > > Right, so you can cache the phys_page and apic lookups. > > A different layer in which to accomplish this is to have a per-device > tlb. So we'd have > > CachedPhysicalAddress msi_fsb_tlb; > > stl_phys_tlb(&msi_fsb_cache, addr, data); > > which caches the addr lookup in a 1-entry tlb. If we have many non-msi > repeated device writes to the same address, this might be worthwhile. > From a quick look at the code, I don't think we do, though. There is no caching need if the messages targets the fixed MSI address window. We can simply pass it directly to the APIC then. Jan
diff --git a/hw/msix.c b/hw/msix.c index 8b68a6a..42870c5 100644 --- a/hw/msix.c +++ b/hw/msix.c @@ -51,7 +51,7 @@ int msix_supported; static void kvm_msix_free(PCIDevice *dev) { int vector, changed = 0; - struct kvm_msix_message *kmm; + KVMMsiMessage *kmm; for (vector = 0; vector < dev->msix_entries_nr; ++vector) { if (dev->msix_entry_used[vector]) { @@ -66,7 +66,7 @@ static void kvm_msix_free(PCIDevice *dev) } static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector, - struct kvm_msix_message *kmm) + KVMMsiMessage *kmm) { uint8_t *table_entry = dev->msix_table_page + vector * MSIX_ENTRY_SIZE; @@ -78,7 +78,7 @@ static void kvm_msix_message_from_vector(PCIDevice *dev, unsigned vector, static void kvm_msix_update(PCIDevice *dev, int vector, int was_masked, int is_masked) { - struct kvm_msix_message e = {}, *entry; + KVMMsiMessage e = {}, *entry; int mask_cleared = was_masked && !is_masked; /* It is only legal to change an entry when it is masked. Therefore, it is * enough to update the routing in kernel when mask is being cleared. */ @@ -114,7 +114,7 @@ static void kvm_msix_update(PCIDevice *dev, int vector, static int kvm_msix_add(PCIDevice *dev, unsigned vector) { - struct kvm_msix_message *kmm = dev->msix_irq_entries + vector; + KVMMsiMessage *kmm = dev->msix_irq_entries + vector; int r; if (!kvm_has_gsi_routing()) { @@ -147,7 +147,7 @@ static int kvm_msix_add(PCIDevice *dev, unsigned vector) static void kvm_msix_del(PCIDevice *dev, unsigned vector) { - struct kvm_msix_message *kmm; + KVMMsiMessage *kmm; if (dev->msix_entry_used[vector]) { return; diff --git a/hw/pci.h b/hw/pci.h index dd09494..dc5df17 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -5,6 +5,7 @@ #include "qobject.h" #include "qdev.h" +#include "kvm.h" /* PCI includes legacy ISA access. */ #include "isa.h" @@ -129,13 +130,6 @@ enum { typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector, int masked); -struct kvm_msix_message { - uint32_t gsi; - uint32_t addr_lo; - uint32_t addr_hi; - uint32_t data; -}; - struct PCIDevice { DeviceState qdev; /* PCI config space */ @@ -208,7 +202,7 @@ struct PCIDevice { * on the rest of the region. */ target_phys_addr_t msix_page_size; - struct kvm_msix_message *msix_irq_entries; + KVMMsiMessage *msix_irq_entries; msix_mask_notifier_func msix_mask_notifier; }; diff --git a/kvm.h b/kvm.h index b890b5d..8ece0b3 100644 --- a/kvm.h +++ b/kvm.h @@ -214,6 +214,13 @@ int kvm_set_irqfd(int gsi, int fd, bool assigned) int kvm_set_ioeventfd_pio_word(int fd, uint16_t adr, uint16_t val, bool assign); +typedef struct KVMMsiMessage { + uint32_t gsi; + uint32_t addr_lo; + uint32_t addr_hi; + uint32_t data; +} KVMMsiMessage; + int kvm_has_gsi_routing(void); int kvm_get_irq_route_gsi(void); int kvm_add_msix(uint32_t gsi, uint32_t addr_lo,