qemu-kvm: Clean up and enhance MSI irqchip support

hw/hpet.c static int deactivating_bit(uint64_t old, uint64_t new, uint64_t mask) · line 149

Quoted 6 lines · Jan Kiszka

 hw/hpet.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-

hw/hpet.c +4 -0

+static int modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
+{
+    return (old ^ new) & mask;
+}

Such constructs always look suspicious. I'm not even sure anymore (after using C for over 20 years ;) that this works... how about

return (old ^ new) & mask ? 1 : 0;

instead, or something along this? I mean, if sizeof(int)==4, how `return 1ULL<<32' will be interpreted in this context? Tiny test program tells me it will return 0...

Quoted 12 lines · Michael Tokarev

Quoted 6 lines · Jan Kiszka

  hw/hpet.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-

+static int modifying_bit(uint64_t old, uint64_t new, uint64_t mask) +{
+ return (old ^ new)& mask;
+}

Such constructs always look suspicious. I'm not even sure anymore (after using C for over 20 years ;) that this works... how about

Quoted 5 lines

  return (old ^ new)&  mask  ? 1 : 0;

instead, or something along this? I mean, if sizeof(int)==4, how `return 1ULL<<32' will be interpreted in this context? Tiny test program tells me it will return 0...

Good catch, will fix (doesn't bite use here, flags fit into 32 bit, but nevertheless).

Quoted 19 lines · Jan Kiszka

Quoted 17 lines · Michael Tokarev

Quoted 6 lines · Jan Kiszka

  hw/hpet.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-

+static int modifying_bit(uint64_t old, uint64_t new, uint64_t mask) +{
+ return (old ^ new)& mask;
+}

Such constructs always look suspicious. I'm not even sure anymore (after using C for over 20 years ;) that this works... how about

  return (old ^ new)&  mask  ? 1 : 0;

instead, or something along this? I mean, if sizeof(int)==4, how `return 1ULL<<32' will be interpreted in this context? Tiny test program tells me it will return 0...

Quoted 3 lines

Good catch, will fix (doesn't bite use here, flags fit into 32 bit, but nevertheless).

Note, a bool return type works here.

hw/hpet.c static int deactivating_bit(uint64_t old, uint64_t new, uint64_t mask) · line 150

Quoted 5 lines · Jan Kiszka

hw/hpet.c +5 -0

+static bool modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
+{
+    return (old ^ new) & mask;
+}
+

A more usual name would be toggle_bit. But you're passing in a mask to be modified, so it would be more a toggle_bits or toggle_mask.

Quoted 10 lines · Christoph Hellwig

hw/hpet.c static int deactivating_bit(uint64_t old, uint64_t new, uint64_t mask) · line 150

Quoted 5 lines · Jan Kiszka

hw/hpet.c +5 -0

+static bool modifying_bit(uint64_t old, uint64_t new, uint64_t mask)
+{
+    return (old ^ new) & mask;
+}
+

A more usual name would be toggle_bit. But you're passing in a mask to be modified, so it would be more a toggle_bits or toggle_mask.

This function is checking for a change. But I've no problem renaming it to toggling_bit or so if that's preferred.

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). Moreover, it contains an additional patch to refresh the MSI IRQ routes after vmload.

Quoted 8 lines · Jan Kiszka

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). Moreover, it contains an additional patch to refresh the MSI IRQ routes after vmload.

Patches 1-8 applied, thanks. I'm not sure about 9 (hpet kvm msi integration) - it seems very intrusive to do this to every msi-supporting device. At least for pci we get all pci devices done in one shot.

We could do this transparently in hw/apic.c. When the message is sent for the first time we look it up, fail, and update the kvm routing entry. Next time the lookup succeeds and we just use KVM_IRQ_LINE, until the message changes and we need to update the irq entry again.

Quoted 10 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). Moreover, it contains an additional patch to refresh the MSI IRQ routes after vmload.

Quoted 4 lines

Patches 1-8 applied, thanks. I'm not sure about 9 (hpet kvm msi integration) - it seems very intrusive to do this to every msi-supporting device. At least for pci we get all pci devices done in one shot.

Right, it is a but intrusive, but I do not see any real alternative.

Quoted 1 line

Quoted 4 lines

We could do this transparently in hw/apic.c. When the message is sent for the first time we look it up, fail, and update the kvm routing entry. Next time the lookup succeeds and we just use KVM_IRQ_LINE, until the message changes and we need to update the irq entry again.

I thought about this, also for PCI devices that aren't assigned or vhost-driven, but we would quickly end up with unused and never freed IRQ routing entries. We still need to track the vector configurations.

What would help at least in the HPET case is a new DELIVER_MSI syscall that completely skips the IRQ routing thing.

Actually, we only need
routing for IRQs that shall be injected directly at kernel level. OTOH, this service would not be available on existing kernels, and we would not be able to simplify the PCI code that way (due to vhost requirements). So I dropped this idea as well and accepted that IRQ routing is the way to go.

Quoted 24 lines · Jan Kiszka

Quoted 10 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). Moreover, it contains an additional patch to refresh the MSI IRQ routes after vmload.

Quoted 4 lines

Patches 1-8 applied, thanks. I'm not sure about 9 (hpet kvm msi integration) - it seems very intrusive to do this to every msi-supporting device. At least for pci we get all pci devices done in one shot.

Right, it is a but intrusive, but I do not see any real alternative.

Quoted 5 lines

We could do this transparently in hw/apic.c. When the message is sent for the first time we look it up, fail, and update the kvm routing entry. Next time the lookup succeeds and we just use KVM_IRQ_LINE, until the message changes and we need to update the irq entry again.

Quoted 3 lines

I thought about this, also for PCI devices that aren't assigned or vhost-driven, but we would quickly end up with unused and never freed IRQ routing entries. We still need to track the vector configurations.

We can simply drop all route entries that are used exclusively in qemu (i.e. not bound to an irqfd) and let the cache rebuild itself.

Quoted 3 lines

What would help at least in the HPET case is a new DELIVER_MSI syscall that completely skips the IRQ routing thing.

It would only help users of 2.6.40 kernels.

Quoted 7 lines

Actually, we only need
routing for IRQs that shall be injected directly at kernel level. OTOH, this service would not be available on existing kernels, and we would not be able to simplify the PCI code that way (due to vhost requirements). So I dropped this idea as well and accepted that IRQ routing is the way to go.

I think that with the cache cleanup as outlined above it can work, no?

Quoted 3 lines

We can simply drop all route entries that are used exclusively in qemu (i.e. not bound to an irqfd) and let the cache rebuild itself.

When should they be dropped?

Quoted 5 lines · Jan Kiszka

Quoted 3 lines

We can simply drop all route entries that are used exclusively in qemu (i.e. not bound to an irqfd) and let the cache rebuild itself.

When should they be dropped?

Whenever we need to allocate a new routing entry, but cannot because it is full.

def kvm_send_msi_message(addr, val):
gsi = route_cache.get((addr, val), None) if gsi is None:
expire_volatile_route_cache_entries_if_full() gsi = alloc_gsi_cache_entry() route_cache[(addr, val)] = gsi update_route_cache()
kvm_irq_line(gsi, 1)
kvm_irq_line(gsi, 0)

The code would have to be in kvm.c, where it can track whether an entry is volatile or persistent.

Quoted 20 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

Quoted 3 lines

We can simply drop all route entries that are used exclusively in qemu (i.e. not bound to an irqfd) and let the cache rebuild itself.

When should they be dropped?

Whenever we need to allocate a new routing entry, but cannot because it is full.

def kvm_send_msi_message(addr, val):
gsi = route_cache.get((addr, val), None) if gsi is None:
expire_volatile_route_cache_entries_if_full() gsi = alloc_gsi_cache_entry() route_cache[(addr, val)] = gsi update_route_cache()
kvm_irq_line(gsi, 1)
kvm_irq_line(gsi, 0)

Quoted 2 lines

The code would have to be in kvm.c, where it can track whether an entry is volatile or persistent.

Yeah, what I forgot is the other side of this caching: Looking up the GSI from the MSI addr/data tuple. That's more complex than the current O(1) way via device.msitable[vector]. Well, we could use some hash table. But is it worth it? IMHO only if we could radically simplify the PCI device hooking as well (HPET is negligible compared to that). But I'm not yet sure if we can given what vhost needs.

Quoted 24 lines · Jan Kiszka

Quoted 20 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

Quoted 3 lines

We can simply drop all route entries that are used exclusively in qemu (i.e. not bound to an irqfd) and let the cache rebuild itself.

When should they be dropped?

Whenever we need to allocate a new routing entry, but cannot because it is full.

def kvm_send_msi_message(addr, val):
gsi = route_cache.get((addr, val), None) if gsi is None:
expire_volatile_route_cache_entries_if_full() gsi = alloc_gsi_cache_entry() route_cache[(addr, val)] = gsi update_route_cache()
kvm_irq_line(gsi, 1)
kvm_irq_line(gsi, 0)

Quoted 2 lines

The code would have to be in kvm.c, where it can track whether an entry is volatile or persistent.

Quoted 6 lines

Yeah, what I forgot is the other side of this caching: Looking up the GSI from the MSI addr/data tuple. That's more complex than the current O(1) way via device.msitable[vector]. Well, we could use some hash table. But is it worth it? IMHO only if we could radically simplify the PCI device hooking as well (HPET is negligible compared to that). But I'm not yet sure if we can given what vhost needs.

A hash table is indeed overcomplicated for this.

How about a replacement for stl_phys() for the MSI case:

(quoted diff) +1 -1

-             stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
+           msi_stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff, 

&timer->msi_cache);

msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)

{
     if (kvm_msi_enabled() && addr & MSI_ADDR_MASK == msi_base_addr) {
          if (cache->addr != addr || cache->data != data) {
              kvm_update_msi_cache(cache, addr, data);
          }
          kvm_irq_line(cache->gsi, 1);
          kvm_irq_line(cache->gsi, 0);
          return;
     }
     stl_phys(addr, data);
}

This bit of code would need to be updated for IOMMU and interrupt remapping,

but at least it means that devices don't need significant change for kvm support. We could also allocate a single gsi for use in hw/apic.c so hacks like using DMA to generate an MSI will work (will be slow, though).

Quoted 19 lines

A hash table is indeed overcomplicated for this.

How about a replacement for stl_phys() for the MSI case:

- stl_phys(timer->fsb>> 32, timer->fsb& 0xffffffff);
+ msi_stl_phys(timer->fsb>> 32, timer->fsb& 0xffffffff,
&timer->msi_cache);

msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)

 {
      if (kvm_msi_enabled()&&  addr&  MSI_ADDR_MASK == msi_base_addr) {
           if (cache->addr != addr || cache->data != data) {
               kvm_update_msi_cache(cache, addr, data);
           }
           kvm_irq_line(cache->gsi, 1);
           kvm_irq_line(cache->gsi, 0);
           return;
      }
      stl_phys(addr, data);

Quoted 1 line

}

I was planning for a MSI short-path anyway. Also for TCG, it's pointless to go through lengthy stl_phys if we know it's supposed to be an MSI message.

Quoted 4 lines

but at least it means that devices don't need significant change for kvm support. We could also allocate a single gsi for use in hw/apic.c so hacks like using DMA to generate an MSI will work (will be slow, though).

Needs some thoughts, maybe it will work. Though, it's not yet clear to me if we can drop the kvm hooks from msi/msix.c and still support vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't think it's worth the effort.

Quoted 21 lines · Jan Kiszka

Quoted 20 lines

A hash table is indeed overcomplicated for this.

How about a replacement for stl_phys() for the MSI case:

- stl_phys(timer->fsb>> 32, timer->fsb& 0xffffffff);
+ msi_stl_phys(timer->fsb>> 32, timer->fsb& 0xffffffff,
&timer->msi_cache);

msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)

 {
      if (kvm_msi_enabled()&&  addr&  MSI_ADDR_MASK == msi_base_addr) {
           if (cache->addr != addr || cache->data != data) {
               kvm_update_msi_cache(cache, addr, data);
           }
           kvm_irq_line(cache->gsi, 1);
           kvm_irq_line(cache->gsi, 0);
           return;
      }
      stl_phys(addr, data);

}

Quoted 3 lines

I was planning for a MSI short-path anyway. Also for TCG, it's pointless to go through lengthy stl_phys if we know it's supposed to be an MSI message.

I don't think tcg will see much benefit; the decoding path through hw/apic.c isn't complicated.

Quoted 6 lines

Quoted 5 lines

but at least it means that devices don't need significant change for kvm support. We could also allocate a single gsi for use in hw/apic.c so hacks like using DMA to generate an MSI will work (will be slow, though).

Quoted 4 lines

Needs some thoughts, maybe it will work. Though, it's not yet clear to me if we can drop the kvm hooks from msi/msix.c and still support vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't think it's worth the effort.

Right. Do we have other users of MSI besides PCI?

Maybe an intermediate solution is to move kvm_hpet_msi_update() (with a neutral name) into msi.c and have hpet call it whenever things change. So hpet.c isn't aware of kvm directly.

Quoted 7 lines

Quoted 4 lines

I was planning for a MSI short-path anyway. Also for TCG, it's pointless to go through lengthy stl_phys if we know it's supposed to be an MSI message.

I don't think tcg will see much benefit; the decoding path through hw/apic.c isn't complicated.

stl_phys itself is non-trivial, e.g. due to phys_page_find.

Quoted 4 lines

Maybe an intermediate solution is to move kvm_hpet_msi_update() (with a neutral name) into msi.c and have hpet call it whenever things change. So hpet.c isn't aware of kvm directly.

Without caching, you need per-vector tracking to refresh or drop routes. That's what the hooks are about.

Intermediate solutions (like hacking msi.c now) are one thing. We also have to know where we can go to long-term.

Quoted 9 lines · Jan Kiszka

Quoted 7 lines

Quoted 4 lines

I was planning for a MSI short-path anyway. Also for TCG, it's pointless to go through lengthy stl_phys if we know it's supposed to be an MSI message.

I don't think tcg will see much benefit; the decoding path through hw/apic.c isn't complicated.

stl_phys itself is non-trivial, e.g. due to phys_page_find.

That is true. But consider that MSI delivery has to emulate interrupt injection through the IDT...

Quoted 7 lines

Quoted 4 lines

Maybe an intermediate solution is to move kvm_hpet_msi_update() (with a neutral name) into msi.c and have hpet call it whenever things change. So hpet.c isn't aware of kvm directly.

Without caching, you need per-vector tracking to refresh or drop routes. That's what the hooks are about.

Right, my proposal doesn't really change your code, it simply moves the kvm specific parts away from hpet.c. Instead of a general cache, the caller is supposed to provide a unique KVMMsiMessage per msi vector, so there is no cache management.

Quoted 2 lines

Intermediate solutions (like hacking msi.c now) are one thing. We also have to know where we can go to long-term.

The really general case (allow generating an MSI via DMA, or allow the device to write to non-MSI addresses via MSI generation) needs the full blown cache. But while I've seen these techniques actually used, I hope they're rare.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 22 lines · Avi Kivity

Quoted 21 lines · Jan Kiszka

Quoted 20 lines

A hash table is indeed overcomplicated for this.

How about a replacement for stl_phys() for the MSI case:

- stl_phys(timer->fsb>> 32, timer->fsb& 0xffffffff);
+ msi_stl_phys(timer->fsb>> 32, timer->fsb& 0xffffffff,
&timer->msi_cache);

msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)

 {
      if (kvm_msi_enabled()&&  addr&  MSI_ADDR_MASK == msi_base_addr) {
           if (cache->addr != addr || cache->data != data) {
               kvm_update_msi_cache(cache, addr, data);
           }
           kvm_irq_line(cache->gsi, 1);
           kvm_irq_line(cache->gsi, 0);
           return;
      }
      stl_phys(addr, data);

}

Quoted 6 lines

Quoted 3 lines

I was planning for a MSI short-path anyway. Also for TCG, it's pointless to go through lengthy stl_phys if we know it's supposed to be an MSI message.

I don't think tcg will see much benefit; the decoding path through hw/apic.c isn't complicated.

stl_phys itself is non-trivial, e.g. due to phys_page_find.

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

but at least it means that devices don't need significant change for kvm support. We could also allocate a single gsi for use in hw/apic.c so hacks like using DMA to generate an MSI will work (will be slow, though).

Needs some thoughts, maybe it will work. Though, it's not yet clear to me if we can drop the kvm hooks from msi/msix.c and still support vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't think it's worth the effort.

Quoted 1 line

Right. Do we have other users of MSI besides PCI?

And HPET? Not yet. IOMMU will become another user. Maybe future chipsets will include more non-PCI devices that issue MSIs.

Quoted 4 lines

Maybe an intermediate solution is to move kvm_hpet_msi_update() (with a neutral name) into msi.c and have hpet call it whenever things change. So hpet.c isn't aware of kvm directly.

Without caching, you need per-vector tracking to refresh or drop routes. That's what the hooks are about.

Intermediate solutions (like hacking msi.c now) are one thing. We also have to know where we can go to long-term.

Jan

Signature · 2 lines

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

Quoted 43 lines · Jan Kiszka

Quoted 22 lines · Avi Kivity

Quoted 21 lines · Jan Kiszka

Quoted 20 lines

A hash table is indeed overcomplicated for this.

How about a replacement for stl_phys() for the MSI case:

- stl_phys(timer->fsb>> 32, timer->fsb& 0xffffffff);
+ msi_stl_phys(timer->fsb>> 32, timer->fsb& 0xffffffff,
&timer->msi_cache);

msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)

 {
      if (kvm_msi_enabled()&&  addr&  MSI_ADDR_MASK == msi_base_addr) {
           if (cache->addr != addr || cache->data != data) {
               kvm_update_msi_cache(cache, addr, data);
           }
           kvm_irq_line(cache->gsi, 1);
           kvm_irq_line(cache->gsi, 0);
           return;
      }
      stl_phys(addr, data);

}

Quoted 6 lines

Quoted 3 lines

I was planning for a MSI short-path anyway. Also for TCG, it's pointless to go through lengthy stl_phys if we know it's supposed to be an MSI message.

I don't think tcg will see much benefit; the decoding path through hw/apic.c isn't complicated.

stl_phys itself is non-trivial, e.g. due to phys_page_find.

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

but at least it means that devices don't need significant change for kvm support. We could also allocate a single gsi for use in hw/apic.c so hacks like using DMA to generate an MSI will work (will be slow, though).

Needs some thoughts, maybe it will work. Though, it's not yet clear to me if we can drop the kvm hooks from msi/msix.c and still support vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't think it's worth the effort.

Quoted 4 lines

Quoted 1 line

Right. Do we have other users of MSI besides PCI?

And HPET? Not yet. IOMMU will become another user. Maybe future chipsets will include more non-PCI devices that issue MSIs.

In theory any device can issue writes into the memory range that apic maps to MSI and I don't think apic can tell what caused this.

Another issue is the reverse: regular memory address can be put in the MSIX/MSI field and the result should be a regular memory write.

Quoted 3 lines

Another issue is the reverse: regular memory address can be put in the MSIX/MSI field and the result should be a regular memory write.

Yes, that's a separate issue: Requests issued by the CPUs have to be told apart from those issued by devices.

Quoted 6 lines · Jan Kiszka

Quoted 3 lines

Another issue is the reverse: regular memory address can be put in the MSIX/MSI field and the result should be a regular memory write.

Yes, that's a separate issue: Requests issued by the CPUs have to be told apart from those issued by devices.

Seems to be the reverse: in qemu msi writes are implemented as regular writes so you can stick a non msi address there and it works. in kvm they are handled differently and that's broken ...

Quoted 34 lines · Avi Kivity

Quoted 24 lines · Jan Kiszka

Quoted 20 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

Quoted 3 lines

We can simply drop all route entries that are used exclusively in qemu (i.e. not bound to an irqfd) and let the cache rebuild itself.

When should they be dropped?

Whenever we need to allocate a new routing entry, but cannot because it is full.

def kvm_send_msi_message(addr, val):
gsi = route_cache.get((addr, val), None) if gsi is None:
expire_volatile_route_cache_entries_if_full() gsi = alloc_gsi_cache_entry() route_cache[(addr, val)] = gsi update_route_cache()
kvm_irq_line(gsi, 1)
kvm_irq_line(gsi, 0)

Quoted 2 lines

The code would have to be in kvm.c, where it can track whether an entry is volatile or persistent.

Quoted 6 lines

Yeah, what I forgot is the other side of this caching: Looking up the GSI from the MSI addr/data tuple. That's more complex than the current O(1) way via device.msitable[vector]. Well, we could use some hash table. But is it worth it? IMHO only if we could radically simplify the PCI device hooking as well (HPET is negligible compared to that). But I'm not yet sure if we can given what vhost needs.

A hash table is indeed overcomplicated for this.

Quoted 7 lines

How about a replacement for stl_phys() for the MSI case:

(quoted diff) +1 -1

-             stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
+           msi_stl_phys(timer->fsb >> 32, timer->fsb & 0xffffffff, 

&timer->msi_cache);

msi_stl_phys(target_phys_addr_t addr, uint32_t data, MSICache *cache)

Quoted 7 lines

{
     if (kvm_msi_enabled() && addr & MSI_ADDR_MASK == msi_base_addr) {
          if (cache->addr != addr || cache->data != data) {
              kvm_update_msi_cache(cache, addr, data);
          }
          kvm_irq_line(cache->gsi, 1);
          kvm_irq_line(cache->gsi, 0);

Quoted 4 lines

          return;
     }
     stl_phys(addr, data);
}

I was planning for a MSI short-path anyway. Also for TCG, it's pointless to go through lengthy stl_phys if we know it's supposed to be an MSI message.

Quoted 3 lines

This bit of code would need to be updated for IOMMU and interrupt remapping,

Quite a bit updates required for that anyway.

Quoted 4 lines

but at least it means that devices don't need significant change for kvm support. We could also allocate a single gsi for use in hw/apic.c so hacks like using DMA to generate an MSI will work (will be slow, though).

Needs some thoughts, maybe it will work. Though, it's not yet clear to me if we can drop the kvm hooks from msi/msix.c and still support vhost/dev-assignment this way. Just to keep hpet.c cleaner, I don't think it's worth the effort.

Jan

Signature · 2 lines

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

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). Moreover, it contains an additional patch to refresh the MSI IRQ routes after vmload.

Quoted 8 lines · Jan Kiszka

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). Moreover, it contains an additional patch to refresh the MSI IRQ routes after vmload.

Immediately after migration:

Program terminated with signal 11, Segmentation fault.
#0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at ../bswap.h:178
178 return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24);
Missing separate debuginfos, use: debuginfo-install SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64
(gdb) bt
#0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at ../bswap.h:178
#1 pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326
#2 kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120
#3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
#4 0x000000000041e29b in get_pci_config_device (f=0x2466380, pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346 #5 0x000000000049c36c in vmstate_load_state (f=0x2466380, vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374 #6 0x000000000049c323 in vmstate_load_state (f=0x2466380, vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372

#7  0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450
#8  qemu_loadvm_state (f=0x2466380) at savevm.c:1817
#9  0x0000000000493e69 in process_incoming_migration (f=<value optimized 

out>) at migration.c:66
#10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value optimized out>) at migration-tcp.c:163
#11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, writefds=0x7fff56dc03b0, xfds=<value optimized out>,
ret=<value optimized out>) at iohandler.c:120
#12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336
#13 0x0000000000433a97 in kvm_main_loop () at /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588
#14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>)

     at /build/home/tlv/akivity/qemu-kvm/vl.c:1369

#15 main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257

(gdb) fr
#3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 (gdb) p dev.msi_irq_entries
$10 = (struct KVMMsiMessage *) 0x0

dev points to the i440fx chipset device.

Yeah, better use this version of patch 8.

Quoted 66 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). Moreover, it contains an additional patch to refresh the MSI IRQ routes after vmload.

Immediately after migration:

Program terminated with signal 11, Segmentation fault.
#0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at ../bswap.h:178
178 return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24);
Missing separate debuginfos, use: debuginfo-install SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64
(gdb) bt
#0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at ../bswap.h:178
#1 pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326
#2 kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120
#3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
#4 0x000000000041e29b in get_pci_config_device (f=0x2466380, pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346 #5 0x000000000049c36c in vmstate_load_state (f=0x2466380, vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374 #6 0x000000000049c323 in vmstate_load_state (f=0x2466380, vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372

#7  0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450
#8  qemu_loadvm_state (f=0x2466380) at savevm.c:1817
#9  0x0000000000493e69 in process_incoming_migration (f=<value optimized 

out>) at migration.c:66
#10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value optimized out>) at migration-tcp.c:163
#11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, writefds=0x7fff56dc03b0, xfds=<value optimized out>,
ret=<value optimized out>) at iohandler.c:120
#12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336
#13 0x0000000000433a97 in kvm_main_loop () at /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588
#14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>)

     at /build/home/tlv/akivity/qemu-kvm/vl.c:1369

#15 main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257

Quoted 8 lines

(gdb) fr
#3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 (gdb) p dev.msi_irq_entries
$10 = (struct KVMMsiMessage *) 0x0

dev points to the i440fx chipset device.

Yeah, better use this version of patch 8.

Jan

----8<-----

Establish a post-load notification for the MSI subsystem so that KVM can refresh its IRQ routing after vmload.

Signed-off-by Jan Kiszka <[email protected]>

---

v3: Fix null-pointer deref after vmload by checking for availability of
msi routing entries.

 hw/msi.c |   13 +++++++++++++
 hw/msi.h |    1 +
 hw/pci.c |    2 ++
 3 files changed, 16 insertions(+), 0 deletions(-)

hw/msi.c +13 -0

index 18f683b..725b2b7 100644
@@ -453,3 +453,16 @@ 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_post_load(PCIDevice *dev)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+
+    if (kvm_enabled() && dev->msi_irq_entries) {
+        kvm_msi_free(dev);
+
+        if (flags & PCI_MSI_FLAGS_ENABLE) {
+            kvm_msi_update(dev);
+        }
+    }
+}

hw/msi.h +1 -0

index 5766018..6ff0607 100644
@@ -32,6 +32,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_post_load(PCIDevice *dev);
 
 static inline bool msi_present(const PCIDevice *dev)
 {

hw/pci.c +1 -0

index 82e0300..07ec4f9 100644
@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

hw/pci.c +0 -0

     qemu_free(config);
     return 0;

Signature · 1 line

1.7.1

Quoted 144 lines · Jan Kiszka

Quoted 66 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). Moreover, it contains an additional patch to refresh the MSI IRQ routes after vmload.

Immediately after migration:

Program terminated with signal 11, Segmentation fault.
#0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at ../bswap.h:178
178 return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24);
Missing separate debuginfos, use: debuginfo-install SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64
(gdb) bt
#0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at ../bswap.h:178
#1 pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326
#2 kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120
#3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
#4 0x000000000041e29b in get_pci_config_device (f=0x2466380, pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346 #5 0x000000000049c36c in vmstate_load_state (f=0x2466380, vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374 #6 0x000000000049c323 in vmstate_load_state (f=0x2466380, vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372

#7  0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450
#8  qemu_loadvm_state (f=0x2466380) at savevm.c:1817
#9  0x0000000000493e69 in process_incoming_migration (f=<value optimized 

out>) at migration.c:66
#10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value optimized out>) at migration-tcp.c:163
#11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, writefds=0x7fff56dc03b0, xfds=<value optimized out>,
ret=<value optimized out>) at iohandler.c:120
#12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336
#13 0x0000000000433a97 in kvm_main_loop () at /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588
#14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>)

     at /build/home/tlv/akivity/qemu-kvm/vl.c:1369

#15 main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257

Quoted 8 lines

(gdb) fr
#3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 (gdb) p dev.msi_irq_entries
$10 = (struct KVMMsiMessage *) 0x0

dev points to the i440fx chipset device.

Yeah, better use this version of patch 8.

Jan

----8<-----

Establish a post-load notification for the MSI subsystem so that KVM can refresh its IRQ routing after vmload.

Signed-off-by Jan Kiszka <[email protected]>

---

v3: Fix null-pointer deref after vmload by checking for availability of
msi routing entries.

 hw/msi.c |   13 +++++++++++++
 hw/msi.h |    1 +
 hw/pci.c |    2 ++
 3 files changed, 16 insertions(+), 0 deletions(-)

hw/msi.c +13 -0

index 18f683b..725b2b7 100644
@@ -453,3 +453,16 @@ 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_post_load(PCIDevice *dev)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+
+    if (kvm_enabled() && dev->msi_irq_entries) {
+        kvm_msi_free(dev);
+
+        if (flags & PCI_MSI_FLAGS_ENABLE) {
+            kvm_msi_update(dev);
+        }
+    }
+}

hw/msi.h +1 -0

index 5766018..6ff0607 100644
@@ -32,6 +32,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_post_load(PCIDevice *dev);
 
 static inline bool msi_present(const PCIDevice *dev)
 {

hw/pci.c +1 -0

index 82e0300..07ec4f9 100644
@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 

hw/pci.c static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) · line 346

Quoted 2 lines

hw/pci.c +1 -0

     pci_update_mappings(s);
+    msi_post_load(s);

Pls don't do this: I'm trying to keep just the core in pci.c and all capabilities in separate files. msix has msix_load, msi will just need one too, and let all devices call that.

Quoted 145 lines · Michael S. Tsirkin

Quoted 144 lines · Jan Kiszka

Quoted 66 lines · Avi Kivity

Quoted 8 lines · Jan Kiszka

I've still plans to consolidate MSI-X mask notifiers and KVM hooks, but that can wait until we go upstream.

This version still makes classic MSI usable in irqchip mode, now not only for PCI devices (AHCI, HDA) but also for the HPET (with msi=on). Moreover, it contains an additional patch to refresh the MSI IRQ routes after vmload.

Immediately after migration:

Program terminated with signal 11, Segmentation fault.
#0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at ../bswap.h:178
178 return p1[0] | (p1[1] << 8) | (p1[2] << 16) | (p1[3] << 24);
Missing separate debuginfos, use: debuginfo-install SDL-1.2.14-10.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 cyrus-sasl-plain-2.1.23-12.fc14.x86_64 db4-4.8.30-2.fc14.x86_64 glibc-2.13-1.x86_64 gnutls-2.8.6-2.fc14.x86_64 keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.2-9.fc14.x86_64 libX11-1.3.4-4.fc14.x86_64 libXau-1.0.6-1.fc14.x86_64 libaio-0.3.109-2.fc13.x86_64 libcom_err-1.41.12-6.fc14.x86_64 libcurl-7.21.0-6.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 libidn-1.18-1.fc14.x86_64 libpng-1.2.44-1.fc14.x86_64 libselinux-2.0.96-6.fc14.1.x86_64 libssh2-1.2.4-1.fc14.x86_64 libtasn1-2.7-1.fc14.x86_64 libxcb-1.7-1.fc14.x86_64 ncurses-libs-5.7-9.20100703.fc14.x86_64 nspr-4.8.7-1.fc14.x86_64 nss-3.12.9-9.fc14.x86_64 nss-softokn-freebl-3.12.9-5.fc14.x86_64 nss-util-3.12.9-1.fc14.x86_64 openldap-2.4.23-4.fc14.x86_64 openssl-1.0.0d-1.fc14.x86_64 zlib-1.2.5-2.fc14.x86_64
(gdb) bt
#0 le32_to_cpupu (vector=0, kmm=0x0, dev=<value optimized out>) at ../bswap.h:178
#1 pci_get_long (vector=0, kmm=0x0, dev=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/hw/pci.h:326
#2 kvm_msi_message_from_vector (vector=0, kmm=0x0, dev=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:120
#3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152
#4 0x000000000041e29b in get_pci_config_device (f=0x2466380, pv=0x23eac28, size=256) at /build/home/tlv/akivity/qemu-kvm/hw/pci.c:346 #5 0x000000000049c36c in vmstate_load_state (f=0x2466380, vmsd=0x5fb880, opaque=0x23eabb0, version_id=2) at savevm.c:1374 #6 0x000000000049c323 in vmstate_load_state (f=0x2466380, vmsd=0x6f07c0, opaque=0x23eabb0, version_id=3) at savevm.c:1372

#7  0x000000000049cf84 in vmstate_load (f=0x2466380) at savevm.c:1450
#8  qemu_loadvm_state (f=0x2466380) at savevm.c:1817
#9  0x0000000000493e69 in process_incoming_migration (f=<value optimized 

out>) at migration.c:66
#10 0x0000000000494b97 in tcp_accept_incoming_migration (opaque=<value optimized out>) at migration-tcp.c:163
#11 0x00000000004a3fa7 in qemu_iohandler_poll (readfds=0x7fff56dc0430, writefds=0x7fff56dc03b0, xfds=<value optimized out>,
ret=<value optimized out>) at iohandler.c:120
#12 0x000000000041944a in main_loop_wait (nonblocking=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:1336
#13 0x0000000000433a97 in kvm_main_loop () at /build/home/tlv/akivity/qemu-kvm/qemu-kvm.c:1588
#14 0x000000000041a3a6 in main_loop (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>)

     at /build/home/tlv/akivity/qemu-kvm/vl.c:1369

#15 main (argc=<value optimized out>, argv=<value optimized out>, envp=<value optimized out>) at /build/home/tlv/akivity/qemu-kvm/vl.c:3257

Quoted 8 lines

(gdb) fr
#3 0x000000000057d59c in kvm_msi_update (dev=0x23eabb0) at /build/home/tlv/akivity/qemu-kvm/hw/msi.c:152 (gdb) p dev.msi_irq_entries
$10 = (struct KVMMsiMessage *) 0x0

dev points to the i440fx chipset device.

Yeah, better use this version of patch 8.

Jan

----8<-----

Establish a post-load notification for the MSI subsystem so that KVM can refresh its IRQ routing after vmload.

Signed-off-by Jan Kiszka <[email protected]>

---

v3: Fix null-pointer deref after vmload by checking for availability of
msi routing entries.

 hw/msi.c |   13 +++++++++++++
 hw/msi.h |    1 +
 hw/pci.c |    2 ++
 3 files changed, 16 insertions(+), 0 deletions(-)

hw/msi.c +13 -0

index 18f683b..725b2b7 100644
@@ -453,3 +453,16 @@ 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_post_load(PCIDevice *dev)
+{
+    uint16_t flags = pci_get_word(dev->config + msi_flags_off(dev));
+
+    if (kvm_enabled() && dev->msi_irq_entries) {
+        kvm_msi_free(dev);
+
+        if (flags & PCI_MSI_FLAGS_ENABLE) {
+            kvm_msi_update(dev);
+        }
+    }
+}

hw/msi.h +1 -0

index 5766018..6ff0607 100644
@@ -32,6 +32,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_post_load(PCIDevice *dev);
 
 static inline bool msi_present(const PCIDevice *dev)
 {

hw/pci.c +1 -0

index 82e0300..07ec4f9 100644
@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 

Quoted 8 lines

hw/pci.c static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) · line 346

Quoted 2 lines

hw/pci.c +1 -0

     pci_update_mappings(s);
+    msi_post_load(s);

Pls don't do this: I'm trying to keep just the core in pci.c and all capabilities in separate files. msix has msix_load, msi will just need one too, and let all devices call that.

Preferred alternatives are...? Registering a vmstate for msi?

Jan

Signature · 2 lines

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

Quoted 22 lines

Quoted 21 lines

Quoted 11 lines

hw/pci.c +1 -0

@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

hw/pci.c static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) · line 346

Quoted 4 lines

hw/pci.c +1 -0

@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 
     pci_update_mappings(s);
+    msi_post_load(s);

Pls don't do this: I'm trying to keep just the core in pci.c and all capabilities in separate files. msix has msix_load, msi will just need one too, and let all devices call that.

Quoted 3 lines

Preferred alternatives are...? Registering a vmstate for msi?

Jan

Add msi_load and call that from devices that need it. Like msix_load does now.

Quoted 26 lines · Michael S. Tsirkin

Quoted 25 lines

Quoted 13 lines

Quoted 13 lines

hw/pci.c +1 -0

@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 

Quoted 8 lines

hw/pci.c static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) · line 346

Quoted 2 lines

hw/pci.c +1 -0

     pci_update_mappings(s);
+    msi_post_load(s);

Pls don't do this: I'm trying to keep just the core in pci.c and all capabilities in separate files. msix has msix_load, msi will just need one too, and let all devices call that.

Preferred alternatives are...? Registering a vmstate for msi?

Jan

Quoted 3 lines

Add msi_load and call that from devices that need it. Like msix_load does now.

msix_load/save are refactoring candidates IMHO. MSI-X has a real need for storing additional state information, so it should register its own subsection.

I don't want to offload this burden to the devices also for MSI.
From the devices' POV, why shouldn't msi_init suffice?

Jan

Signature · 2 lines

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

Quoted 27 lines · Jan Kiszka

Quoted 26 lines · Michael S. Tsirkin

Quoted 25 lines

Quoted 13 lines

Quoted 13 lines

hw/pci.c +1 -0

@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 

Quoted 8 lines

hw/pci.c static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) · line 346

Quoted 2 lines

hw/pci.c +1 -0

     pci_update_mappings(s);
+    msi_post_load(s);

Pls don't do this: I'm trying to keep just the core in pci.c and all capabilities in separate files. msix has msix_load, msi will just need one too, and let all devices call that.

Preferred alternatives are...? Registering a vmstate for msi?

Jan

Quoted 7 lines

Quoted 3 lines

Add msi_load and call that from devices that need it. Like msix_load does now.

msix_load/save are refactoring candidates IMHO. MSI-X has a real need for storing additional state information, so it should register its own subsection.

That's an implementation detail though, isn't it.

Quoted 5 lines

I don't want to offload this burden to the devices also for MSI.
From the devices' POV, why shouldn't msi_init suffice?

Jan

One can also claim this about config writes:

    pci_bridge_write_config(d, address, val, len);
    pcie_cap_flr_write_config(d, address, val, len);
    pcie_cap_slot_write_config(d, address, val, len);
    msi_write_config(d, address, val, len);
    pcie_aer_write_config(d, address, val, len);

which arguably just duplicates the initialization sequence.

What I'm trying to do though is to keep it modular and keep module inter-dependencies to a minimum, so that pci is the core and msix depends on it but not the other way around.

Quoted 50 lines · Michael S. Tsirkin

Quoted 27 lines · Jan Kiszka

Quoted 26 lines · Michael S. Tsirkin

Quoted 25 lines

Quoted 13 lines

Quoted 13 lines

hw/pci.c +1 -0

@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 

Quoted 8 lines

hw/pci.c static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) · line 346

Quoted 2 lines

hw/pci.c +1 -0

     pci_update_mappings(s);
+    msi_post_load(s);

Pls don't do this: I'm trying to keep just the core in pci.c and all capabilities in separate files. msix has msix_load, msi will just need one too, and let all devices call that.

Preferred alternatives are...? Registering a vmstate for msi?

Jan

Quoted 7 lines

Quoted 3 lines

Add msi_load and call that from devices that need it. Like msix_load does now.

msix_load/save are refactoring candidates IMHO. MSI-X has a real need for storing additional state information, so it should register its own subsection.

That's an implementation detail though, isn't it.

Quoted 5 lines

I don't want to offload this burden to the devices also for MSI.
From the devices' POV, why shouldn't msi_init suffice?

Jan

One can also claim this about config writes:

    pci_bridge_write_config(d, address, val, len);
    pcie_cap_flr_write_config(d, address, val, len);
    pcie_cap_slot_write_config(d, address, val, len);
    msi_write_config(d, address, val, len);
    pcie_aer_write_config(d, address, val, len);

Quoted 6 lines

which arguably just duplicates the initialization sequence.

What I'm trying to do though is to keep it modular and keep module inter-dependencies to a minimum, so that pci is the core and msix depends on it but not the other way around.

I still don't see the bigger benefit in saving a single bidirectional dependency at core level vs. saving additional callbacks at each and every MSI user. The latter is also a source for bugs.

Jan

Quoted 4 lines

What I think we should do is to add a pci subdirectory, move all of the stuff there, move pci.c to pci/core.c and
add a high level module that depends on them all
and deals with all the capabilities.

Signature · 2 lines

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

Quoted 58 lines · Jan Kiszka

Quoted 50 lines · Michael S. Tsirkin

Quoted 27 lines · Jan Kiszka

Quoted 26 lines · Michael S. Tsirkin

Quoted 25 lines

Quoted 13 lines

Quoted 13 lines

hw/pci.c +1 -0

@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 

Quoted 8 lines

hw/pci.c static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) · line 346

Quoted 2 lines

hw/pci.c +1 -0

     pci_update_mappings(s);
+    msi_post_load(s);

Pls don't do this: I'm trying to keep just the core in pci.c and all capabilities in separate files. msix has msix_load, msi will just need one too, and let all devices call that.

Preferred alternatives are...? Registering a vmstate for msi?

Jan

Quoted 7 lines

Quoted 3 lines

Add msi_load and call that from devices that need it. Like msix_load does now.

msix_load/save are refactoring candidates IMHO. MSI-X has a real need for storing additional state information, so it should register its own subsection.

That's an implementation detail though, isn't it.

Quoted 5 lines

I don't want to offload this burden to the devices also for MSI.
From the devices' POV, why shouldn't msi_init suffice?

Jan

One can also claim this about config writes:

    pci_bridge_write_config(d, address, val, len);
    pcie_cap_flr_write_config(d, address, val, len);
    pcie_cap_slot_write_config(d, address, val, len);
    msi_write_config(d, address, val, len);
    pcie_aer_write_config(d, address, val, len);

Quoted 6 lines

which arguably just duplicates the initialization sequence.

What I'm trying to do though is to keep it modular and keep module inter-dependencies to a minimum, so that pci is the core and msix depends on it but not the other way around.

Quoted 5 lines

I still don't see the bigger benefit in saving a single bidirectional dependency at core level vs. saving additional callbacks at each and every MSI user. The latter is also a source for bugs.

Jan

Yes but let us be consistent with how e.g. config writes are handled. As I said

Quoted 4 lines

Quoted 4 lines

What I think we should do is to add a pci subdirectory, move all of the stuff there, move pci.c to pci/core.c and
add a high level module that depends on them all
and deals with all the capabilities.

Which will solve both issues without need for tradeoffs.

Quoted 73 lines · Michael S. Tsirkin

Quoted 58 lines · Jan Kiszka

Quoted 50 lines · Michael S. Tsirkin

Quoted 27 lines · Jan Kiszka

Quoted 26 lines · Michael S. Tsirkin

Quoted 25 lines

Quoted 13 lines

Quoted 13 lines

hw/pci.c +1 -0

@@ -34,6 +34,7 @@
 #include "device-assignment.h"
 #include "qemu-objects.h"
 #include "range.h"
+#include "msi.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI

@@ -342,6 +343,7 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
     memcpy(s->config, config, size);
 

Quoted 8 lines

hw/pci.c static int get_pci_config_device(QEMUFile *f, void *pv, size_t size) · line 346

Quoted 2 lines

hw/pci.c +1 -0

     pci_update_mappings(s);
+    msi_post_load(s);

Pls don't do this: I'm trying to keep just the core in pci.c and all capabilities in separate files. msix has msix_load, msi will just need one too, and let all devices call that.

Preferred alternatives are...? Registering a vmstate for msi?

Jan

Quoted 7 lines

Quoted 3 lines

Add msi_load and call that from devices that need it. Like msix_load does now.

msix_load/save are refactoring candidates IMHO. MSI-X has a real need for storing additional state information, so it should register its own subsection.

That's an implementation detail though, isn't it.

Quoted 5 lines

I don't want to offload this burden to the devices also for MSI.
From the devices' POV, why shouldn't msi_init suffice?

Jan

One can also claim this about config writes:

    pci_bridge_write_config(d, address, val, len);
    pcie_cap_flr_write_config(d, address, val, len);
    pcie_cap_slot_write_config(d, address, val, len);
    msi_write_config(d, address, val, len);
    pcie_aer_write_config(d, address, val, len);

Quoted 6 lines

which arguably just duplicates the initialization sequence.

What I'm trying to do though is to keep it modular and keep module inter-dependencies to a minimum, so that pci is the core and msix depends on it but not the other way around.

Quoted 5 lines

I still don't see the bigger benefit in saving a single bidirectional dependency at core level vs. saving additional callbacks at each and every MSI user. The latter is also a source for bugs.

Jan

Yes but let us be consistent with how e.g. config writes are handled. As I said

Quoted 4 lines

Quoted 4 lines

What I think we should do is to add a pci subdirectory, move all of the stuff there, move pci.c to pci/core.c and
add a high level module that depends on them all
and deals with all the capabilities.

Quoted 1 line

Which will solve both issues without need for tradeoffs.

OK, but this patch is not about doing a pci refactoring. And it should not touch any device without a need. So what is your suggestion again?

Jan

Signature · 2 lines

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