| Message ID | 1381762577-12526-43-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: > - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, > - struct pci_status), > + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, > + vmstate_test_no_use_acpi_pci_hotplug, > + 2, vmstate_pci_status, > + struct pci_status), There's no reason to remove this from the stream when a new machine type is in use. You'll just send out zeroes. > + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState, > + vmstate_test_use_acpi_pci_hotplug), This works, but it is a bit different from other cases that are already present, which use a subsection. It is a bit ugly because it looks like a version-1 field, but in fact it is not version 1. I'll let other people decide whether it's acceptable or not, but I'm leaning towards asking you to use a subsection. Paolo
On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote: > Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: > > - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, > > - struct pci_status), > > + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, > > + vmstate_test_no_use_acpi_pci_hotplug, > > + 2, vmstate_pci_status, > > + struct pci_status), > > There's no reason to remove this from the stream when a new machine type > is in use. You'll just send out zeroes. Seemed cleaner not to. > > + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState, > > + vmstate_test_use_acpi_pci_hotplug), > > This works, but it is a bit different from other cases that are already > present, which use a subsection. It is a bit ugly because it looks like > a version-1 field, but in fact it is not version 1. > > I'll let other people decide whether it's acceptable or not, but I'm > leaning towards asking you to use a subsection. > > Paolo
Il 15/10/2013 16:35, Michael S. Tsirkin ha scritto: > On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote: >> Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: >>> - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, >>> - struct pci_status), >>> + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, >>> + vmstate_test_no_use_acpi_pci_hotplug, >>> + 2, vmstate_pci_status, >>> + struct pci_status), >> >> There's no reason to remove this from the stream when a new machine type >> is in use. You'll just send out zeroes. > > Seemed cleaner not to. It certainly would be if we had a self-descriptive migration stream format. However, what we have is "send bytes, parse them on the destination, hope the format matches". Hence, anything that makes the format less declarative adds to the complication and complicates debugging. This is the same reason why I prefer a subsection for the new hotplug stuff---it makes the format more declarative and self-descriptive. Paolo >>> + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState, >>> + vmstate_test_use_acpi_pci_hotplug), >> >> This works, but it is a bit different from other cases that are already >> present, which use a subsection. It is a bit ugly because it looks like >> a version-1 field, but in fact it is not version 1. >> >> I'll let other people decide whether it's acceptable or not, but I'm >> leaning towards asking you to use a subsection. >> >> Paolo
Il 15/10/2013 16:54, Michael S. Tsirkin ha scritto: > > > Seemed cleaner not to. > > > > It certainly would be if we had a self-descriptive migration stream format. > > > > However, what we have is "send bytes, parse them on the destination, > > hope the format matches". Hence, anything that makes the format less > > declarative adds to the complication and complicates debugging. This is > > the same reason why I prefer a subsection for the new hotplug stuff---it > > makes the format more declarative and self-descriptive. > > I understand for the subsection but why is removing > useless bytes from there making it less descriptive? Because the same version can have multiple formats depending on a datum that is not part of the migration stream. Paolo
On Tue, Oct 15, 2013 at 04:50:24PM +0200, Paolo Bonzini wrote: > Il 15/10/2013 16:35, Michael S. Tsirkin ha scritto: > > On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote: > >> Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: > >>> - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, > >>> - struct pci_status), > >>> + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, > >>> + vmstate_test_no_use_acpi_pci_hotplug, > >>> + 2, vmstate_pci_status, > >>> + struct pci_status), > >> > >> There's no reason to remove this from the stream when a new machine type > >> is in use. You'll just send out zeroes. > > > > Seemed cleaner not to. > > It certainly would be if we had a self-descriptive migration stream format. > > However, what we have is "send bytes, parse them on the destination, > hope the format matches". Hence, anything that makes the format less > declarative adds to the complication and complicates debugging. This is > the same reason why I prefer a subsection for the new hotplug stuff---it > makes the format more declarative and self-descriptive. > > Paolo I understand for the subsection but why is removing useless bytes from there making it less descriptive? > >>> + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState, > >>> + vmstate_test_use_acpi_pci_hotplug), > >> > >> This works, but it is a bit different from other cases that are already > >> present, which use a subsection. It is a bit ugly because it looks like > >> a version-1 field, but in fact it is not version 1. > >> > >> I'll let other people decide whether it's acceptable or not, but I'm > >> leaning towards asking you to use a subsection. > >> > >> Paolo
On Tue, Oct 15, 2013 at 04:54:24PM +0200, Paolo Bonzini wrote: > Il 15/10/2013 16:54, Michael S. Tsirkin ha scritto: > > > > Seemed cleaner not to. > > > > > > It certainly would be if we had a self-descriptive migration stream format. > > > > > > However, what we have is "send bytes, parse them on the destination, > > > hope the format matches". Hence, anything that makes the format less > > > declarative adds to the complication and complicates debugging. This is > > > the same reason why I prefer a subsection for the new hotplug stuff---it > > > makes the format more declarative and self-descriptive. > > > > I understand for the subsection but why is removing > > useless bytes from there making it less descriptive? > > Because the same version can have multiple formats depending on a datum > that is not part of the migration stream. > > Paolo I see. Will it be OK if I'll update the version?
Il 15/10/2013 17:07, Michael S. Tsirkin ha scritto: > > Because the same version can have multiple formats depending on a datum > > that is not part of the migration stream. > > I see. Will it be OK if I'll update the version? The version is constant, you cannot change it depending on which PCI flavor you have. If you use subsection (present if and only if new-style hotplug is active) and leave pci0_hotplug always there it's okay. Paolo
On Tue, Oct 15, 2013 at 05:09:36PM +0200, Paolo Bonzini wrote: > Il 15/10/2013 17:07, Michael S. Tsirkin ha scritto: > > > Because the same version can have multiple formats depending on a datum > > > that is not part of the migration stream. > > > > I see. Will it be OK if I'll update the version? > > The version is constant, you cannot change it depending on which PCI > flavor you have. If you use subsection (present if and only if > new-style hotplug is active) and leave pci0_hotplug always there it's okay. > > Paolo OK I can do that, thanks.
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 15/10/2013 16:35, Michael S. Tsirkin ha scritto: >> On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote: >>> Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: >>>> - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, >>>> - struct pci_status), >>>> + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, >>>> + vmstate_test_no_use_acpi_pci_hotplug, >>>> + 2, vmstate_pci_status, >>>> + struct pci_status), >>> >>> There's no reason to remove this from the stream when a new machine type >>> is in use. You'll just send out zeroes. >> >> Seemed cleaner not to. > > It certainly would be if we had a self-descriptive migration stream > format. Yes, removing tests is always a good thing. But I think subsections should always be used when they can. We should not break compatibility (even if we technical don't guarantee it) unless we absolutely have to. Regards, Anthony Liguori > > However, what we have is "send bytes, parse them on the destination, > hope the format matches". Hence, anything that makes the format less > declarative adds to the complication and complicates debugging. This is > the same reason why I prefer a subsection for the new hotplug stuff---it > makes the format more declarative and self-descriptive. > > Paolo > >>>> + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState, >>>> + vmstate_test_use_acpi_pci_hotplug), >>> >>> This works, but it is a bit different from other cases that are already >>> present, which use a subsection. It is a bit ugly because it looks like >>> a version-1 field, but in fact it is not version 1. >>> >>> I'll let other people decide whether it's acceptable or not, but I'm >>> leaning towards asking you to use a subsection. >>> >>> Paolo
On Tue, Oct 15, 2013 at 09:27:33AM -0700, Anthony Liguori wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > Il 15/10/2013 16:35, Michael S. Tsirkin ha scritto: > >> On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote: > >>> Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: > >>>> - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, > >>>> - struct pci_status), > >>>> + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, > >>>> + vmstate_test_no_use_acpi_pci_hotplug, > >>>> + 2, vmstate_pci_status, > >>>> + struct pci_status), > >>> > >>> There's no reason to remove this from the stream when a new machine type > >>> is in use. You'll just send out zeroes. > >> > >> Seemed cleaner not to. > > > > It certainly would be if we had a self-descriptive migration stream > > format. > > Yes, removing tests is always a good thing. > > But I think subsections should always be used when they can. We should > not break compatibility (even if we technical don't guarantee it) unless > we absolutely have to. > > Regards, > > Anthony Liguori OK so I can interpret this in 2 ways wrt bridge hotplug: - it's in shape for 1.7 except the migration which should use subsections (and needs cross-version testing) - it's not in shape for 1.7 Can you tell me which it is please? If it's 1 I'll repost a version with tweaked migration format and cross-version testing report, tomorrow. If it's 2 I'd rather work on this after the forum, I still need to make some travel arrangements. Paolo? Anthony?
Il 15/10/2013 22:17, Michael S. Tsirkin ha scritto: > OK so I can interpret this in 2 ways wrt bridge hotplug: > - it's in shape for 1.7 except the migration which should use > subsections (and needs cross-version testing) > - it's not in shape for 1.7 > > Can you tell me which it is please? > > If it's 1 I'll repost a version with tweaked > migration format and cross-version testing report, tomorrow. > > If it's 2 I'd rather work on this after the forum, I still > need to make some travel arrangements. > > Paolo? Anthony? I honestly didn't look at the patches too much except now that Anthony and you were discussing migration, and won't claim I understand them well. But if you want my opinion, I'll trust you that the spec makes sense and I'll say that for me it's (1). Paolo
On Tue, Oct 15, 2013 at 1:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Oct 15, 2013 at 09:27:33AM -0700, Anthony Liguori wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> > Il 15/10/2013 16:35, Michael S. Tsirkin ha scritto: >> >> On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote: >> >>> Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: >> >>>> - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, >> >>>> - struct pci_status), >> >>>> + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, >> >>>> + vmstate_test_no_use_acpi_pci_hotplug, >> >>>> + 2, vmstate_pci_status, >> >>>> + struct pci_status), >> >>> >> >>> There's no reason to remove this from the stream when a new machine type >> >>> is in use. You'll just send out zeroes. >> >> >> >> Seemed cleaner not to. >> > >> > It certainly would be if we had a self-descriptive migration stream >> > format. >> >> Yes, removing tests is always a good thing. >> >> But I think subsections should always be used when they can. We should >> not break compatibility (even if we technical don't guarantee it) unless >> we absolutely have to. >> >> Regards, >> >> Anthony Liguori > > OK so I can interpret this in 2 ways wrt bridge hotplug: > - it's in shape for 1.7 except the migration which should use > subsections (and needs cross-version testing) > - it's not in shape for 1.7 > > Can you tell me which it is please? The code is not in shape. Forget about the existence of 1.7. Focus on getting the code right and it will make whatever release it is ready for. If that's 1.7, great, but the fact that 1.7 is around the corner does not mean we're going to merge something that isn't ready just so it makes the release. Migration is one issue. As I said before, testing is another. There really should be some test automation for this. Regards, Anthony Liguori > If it's 1 I'll repost a version with tweaked > migration format and cross-version testing report, tomorrow. > > If it's 2 I'd rather work on this after the forum, I still > need to make some travel arrangements. > > Paolo? Anthony? > > -- > MST
On Wed, Oct 16, 2013 at 09:38:29AM -0700, Anthony Liguori wrote: > On Tue, Oct 15, 2013 at 1:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Oct 15, 2013 at 09:27:33AM -0700, Anthony Liguori wrote: > >> Paolo Bonzini <pbonzini@redhat.com> writes: > >> > >> > Il 15/10/2013 16:35, Michael S. Tsirkin ha scritto: > >> >> On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote: > >> >>> Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: > >> >>>> - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, > >> >>>> - struct pci_status), > >> >>>> + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, > >> >>>> + vmstate_test_no_use_acpi_pci_hotplug, > >> >>>> + 2, vmstate_pci_status, > >> >>>> + struct pci_status), > >> >>> > >> >>> There's no reason to remove this from the stream when a new machine type > >> >>> is in use. You'll just send out zeroes. > >> >> > >> >> Seemed cleaner not to. > >> > > >> > It certainly would be if we had a self-descriptive migration stream > >> > format. > >> > >> Yes, removing tests is always a good thing. > >> > >> But I think subsections should always be used when they can. We should > >> not break compatibility (even if we technical don't guarantee it) unless > >> we absolutely have to. > >> > >> Regards, > >> > >> Anthony Liguori > > > > OK so I can interpret this in 2 ways wrt bridge hotplug: > > - it's in shape for 1.7 except the migration which should use > > subsections (and needs cross-version testing) > > - it's not in shape for 1.7 > > > > Can you tell me which it is please? > > The code is not in shape. Forget about the existence of 1.7. Focus > on getting the code right and it will make whatever release it is > ready for. If that's 1.7, great, but the fact that 1.7 is around the > corner does not mean we're going to merge something that isn't ready > just so it makes the release. OK. Apropos 1.7, how about moving soft freeze and the release out by a couple of weeks? What with you moving over and the kvm forum, people didn't have time to focus on it properly IMO. In particular it's harder than usual to get reviews. > Migration is one issue. Right but what's special about this feature? Almost anything we do affects migration in some way. > As I said before, testing is another. There > really should be some test automation for this. > > Regards, > > Anthony Liguori I'm not sure I understand what you are saying here. If you just want testing hotplug, automation is there. Automated testing for cross-version migration? that's not easy since you need two versions around. I'll talk to autotest guys but don't hold your breath. But a bigger issue is that migration and hotplug don't work well together in qemu ATM.
On Wed, Oct 16, 2013 at 11:18 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Oct 16, 2013 at 09:38:29AM -0700, Anthony Liguori wrote: >> On Tue, Oct 15, 2013 at 1:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Tue, Oct 15, 2013 at 09:27:33AM -0700, Anthony Liguori wrote: >> >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> >> >> > Il 15/10/2013 16:35, Michael S. Tsirkin ha scritto: >> >> >> On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote: >> >> >>> Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: >> >> >>>> - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, >> >> >>>> - struct pci_status), >> >> >>>> + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, >> >> >>>> + vmstate_test_no_use_acpi_pci_hotplug, >> >> >>>> + 2, vmstate_pci_status, >> >> >>>> + struct pci_status), >> >> >>> >> >> >>> There's no reason to remove this from the stream when a new machine type >> >> >>> is in use. You'll just send out zeroes. >> >> >> >> >> >> Seemed cleaner not to. >> >> > >> >> > It certainly would be if we had a self-descriptive migration stream >> >> > format. >> >> >> >> Yes, removing tests is always a good thing. >> >> >> >> But I think subsections should always be used when they can. We should >> >> not break compatibility (even if we technical don't guarantee it) unless >> >> we absolutely have to. >> >> >> >> Regards, >> >> >> >> Anthony Liguori >> > >> > OK so I can interpret this in 2 ways wrt bridge hotplug: >> > - it's in shape for 1.7 except the migration which should use >> > subsections (and needs cross-version testing) >> > - it's not in shape for 1.7 >> > >> > Can you tell me which it is please? >> >> The code is not in shape. Forget about the existence of 1.7. Focus >> on getting the code right and it will make whatever release it is >> ready for. If that's 1.7, great, but the fact that 1.7 is around the >> corner does not mean we're going to merge something that isn't ready >> just so it makes the release. > > OK. Apropos 1.7, how about moving soft freeze and the release out > by a couple of weeks? No. There is always some reason to delay releases. We have a release every quarter. It's not a big deal to just wait for a feature for the next release. That's the whole point of doing frequent releases. > What with you moving over and the kvm forum, people > didn't have time to focus on it properly IMO. > In particular it's harder than usual to get reviews. > >> Migration is one issue. > > Right but what's special about this feature? > Almost anything we do affects migration in some way. There is nothing special and the feedback you are getting is no different than any other series. > >> As I said before, testing is another. There >> really should be some test automation for this. >> >> Regards, >> >> Anthony Liguori > > I'm not sure I understand what you are saying here. > > If you just want testing hotplug, automation is there. Unit level testing. IOW, something that gets run during 'make check' to verify that we're generating proper ACPI tables. Regards, Anthony Liguori > Automated testing for cross-version migration? that's not easy since you > need two versions around. I'll talk to autotest guys but don't hold your > breath. But a bigger issue is that migration and hotplug don't work > well together in qemu ATM. > > -- > MST
On Wed, Oct 16, 2013 at 11:18:42AM -0700, Anthony Liguori wrote: > On Wed, Oct 16, 2013 at 11:18 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Oct 16, 2013 at 09:38:29AM -0700, Anthony Liguori wrote: > >> On Tue, Oct 15, 2013 at 1:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > On Tue, Oct 15, 2013 at 09:27:33AM -0700, Anthony Liguori wrote: > >> >> Paolo Bonzini <pbonzini@redhat.com> writes: > >> >> > >> >> > Il 15/10/2013 16:35, Michael S. Tsirkin ha scritto: > >> >> >> On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote: > >> >> >>> Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto: > >> >> >>>> - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, > >> >> >>>> - struct pci_status), > >> >> >>>> + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, > >> >> >>>> + vmstate_test_no_use_acpi_pci_hotplug, > >> >> >>>> + 2, vmstate_pci_status, > >> >> >>>> + struct pci_status), > >> >> >>> > >> >> >>> There's no reason to remove this from the stream when a new machine type > >> >> >>> is in use. You'll just send out zeroes. > >> >> >> > >> >> >> Seemed cleaner not to. > >> >> > > >> >> > It certainly would be if we had a self-descriptive migration stream > >> >> > format. > >> >> > >> >> Yes, removing tests is always a good thing. > >> >> > >> >> But I think subsections should always be used when they can. We should > >> >> not break compatibility (even if we technical don't guarantee it) unless > >> >> we absolutely have to. > >> >> > >> >> Regards, > >> >> > >> >> Anthony Liguori > >> > > >> > OK so I can interpret this in 2 ways wrt bridge hotplug: > >> > - it's in shape for 1.7 except the migration which should use > >> > subsections (and needs cross-version testing) > >> > - it's not in shape for 1.7 > >> > > >> > Can you tell me which it is please? > >> > >> The code is not in shape. Forget about the existence of 1.7. Focus > >> on getting the code right and it will make whatever release it is > >> ready for. If that's 1.7, great, but the fact that 1.7 is around the > >> corner does not mean we're going to merge something that isn't ready > >> just so it makes the release. > > > > OK. Apropos 1.7, how about moving soft freeze and the release out > > by a couple of weeks? > > No. > > There is always some reason to delay releases. We have a release > every quarter. It's not a big deal to just wait for a feature for the > next release. That's the whole point of doing frequent releases. > > > What with you moving over and the kvm forum, people > > didn't have time to focus on it properly IMO. > > In particular it's harder than usual to get reviews. > > > >> Migration is one issue. > > > > Right but what's special about this feature? > > Almost anything we do affects migration in some way. > > There is nothing special and the feedback you are getting is no > different than any other series. > > > > >> As I said before, testing is another. There > >> really should be some test automation for this. > >> > >> Regards, > >> > >> Anthony Liguori > > > > I'm not sure I understand what you are saying here. > > > > If you just want testing hotplug, automation is there. > > Unit level testing. IOW, something that gets run during 'make check' > to verify that we're generating proper ACPI tables. > > Regards, > > Anthony Liguori In particular I do *not* want to write a consumer for the guest interfaces in qemu. bios has one, duplicating it in qemu is not a good idea. By far the best way to test this is to boot some guest, download tables, then run iasl -d on them. I used linux guest but that's not necessary, we could do this by integrating kvm unit test code. But if we do integrate it, I think it's better to merge than fork. Gleb, Paolo, what do you think? OK to merge kvm unit test into qemu? It depends on qemu anyway, in-tree will make it easier. Maybe someone's looking at this already? If people agree I'll try to make sure this happens but I hope it's not a precondition, it's a lot of work (and it won't help if I'm distracted rebasing patches :) ) > > Automated testing for cross-version migration? that's not easy since you > > need two versions around. I'll talk to autotest guys but don't hold your > > breath. But a bigger issue is that migration and hotplug don't work > > well together in qemu ATM. > > > > -- > > MST
Il 16/10/2013 20:37, Michael S. Tsirkin ha scritto: > Gleb, Paolo, what do you think? OK to merge kvm unit test > into qemu? It depends on qemu anyway, in-tree will make it easier. > Maybe someone's looking at this already? I think merging KVM unit tests doesn't make much sense because, with some small exceptions, it is mostly a test or a benchmark for KVM. What may make sense is to have a quick way to run autotest on a QEMU tree, with a subset of testcases that doesn't take too much time (let's say <4 hours) and is more or less guaranteed to pass. KVM unit tests are run by autotest, that should be enough. I agree with Anthony that device model code should be tested by qtest. I'm not sure this extends to firmware interfaces, though, for two reasons: (1) any testcase you could have written would have likely not shown the kind of problem that Igor and Gerd found in your previous versions. Black box unit testing can only do so much for something as complex as a DSDT, while black box integration testing works well. (2) IMO qtest's main advantage is that, at least in principle, the same testcases could run on all the rarely-used almost-unmaintained targets (the endianness-test already does that for example). This does not apply to most firmware interfaces, though. By the way, this advantage of qtest is also being mostly negated by the immaturity (or sheer absence) of infrastructure. Looking at bugs that were reported, at least these two from Igor are probably best handled with integration tests (like autotest or Anthony's qemu-test): * WS2008R2x64 BSODs with ACPI error on boot when 64bit PCI hole is present, but it boots fine with upstream QEMU * hotadd CPU to guest, reboot guest, only initial CPUs are visible to guest qtest could at best host some sanity checks on the ACPI tables, which would catch the MCFG problems that Gerd reported on v5. Gerd also reported some segfaults, not sure how they escaped mst's testing so I cannot judge what kind of testing could have exposed them preemptively. Paolo
On Wed, Oct 16, 2013 at 11:26:11PM +0200, Paolo Bonzini wrote: > Il 16/10/2013 20:37, Michael S. Tsirkin ha scritto: > > Gleb, Paolo, what do you think? OK to merge kvm unit test > > into qemu? It depends on qemu anyway, in-tree will make it easier. > > Maybe someone's looking at this already? > > I think merging KVM unit tests doesn't make much sense because, with > some small exceptions, it is mostly a test or a benchmark for KVM. But why keep them separate? They need qemu to work, don't they? What I wanted to use from kvm unit test is the infrastructure to generating the kernel binary for qemu. > What > may make sense is to have a quick way to run autotest on a QEMU tree, > with a subset of testcases that doesn't take too much time (let's say <4 > hours) That's not really reasonable for make check though. > and is more or less guaranteed to pass. That's still the main challenge. > KVM unit tests are run > by autotest, that should be enough. > > I agree with Anthony that device model code should be tested by qtest. > I'm not sure this extends to firmware interfaces, though, for two reasons: > > (1) any testcase you could have written would have likely not shown the > kind of problem that Igor and Gerd found in your previous versions. > Black box unit testing can only do so much for something as complex as a > DSDT, while black box integration testing works well. > > (2) IMO qtest's main advantage is that, at least in principle, the same > testcases could run on all the rarely-used almost-unmaintained targets > (the endianness-test already does that for example). This does not > apply to most firmware interfaces, though. > > > By the way, this advantage of qtest is also being mostly negated by the > immaturity (or sheer absence) of infrastructure. > Looking at bugs that were reported, at least these two from Igor are > probably best handled with integration tests (like autotest or Anthony's > qemu-test): > > * WS2008R2x64 BSODs with ACPI error on boot when 64bit PCI hole is > present, but it boots fine with upstream QEMU > > * hotadd CPU to guest, reboot guest, only initial CPUs are visible to guest > > > qtest could at best host some sanity checks on the ACPI tables, which > would catch the MCFG problems that Gerd reported on v5. Depends on how deep the test understands ACPI - the signature was wrong I think. Note I was testing this too - comparing tables between revisions. I just didn't notice that list of tables to test included was generated by me on piix, so MCFG wasn't tested. > Gerd also reported some segfaults, not sure how they escaped mst's > testing so I cannot judge what kind of testing could have exposed them > preemptively. > > Paolo Mostly forgot to commit mistakes. I since added a script that checks my tree is clean before build.
Il 17/10/2013 00:03, Michael S. Tsirkin ha scritto: > On Wed, Oct 16, 2013 at 11:26:11PM +0200, Paolo Bonzini wrote: >> Il 16/10/2013 20:37, Michael S. Tsirkin ha scritto: >>> Gleb, Paolo, what do you think? OK to merge kvm unit test >>> into qemu? It depends on qemu anyway, in-tree will make it easier. >>> Maybe someone's looking at this already? >> >> I think merging KVM unit tests doesn't make much sense because, with >> some small exceptions, it is mostly a test or a benchmark for KVM. > > But why keep them separate? They need qemu to work, don't they? Not necessarily. They need a userspace component of course, but most of them do not need something as big as QEMU. Most tests, perhaps all, only write to a handful of ports and use no BIOS services. >> What >> may make sense is to have a quick way to run autotest on a QEMU tree, >> with a subset of testcases that doesn't take too much time (let's say <4 >> hours) > > That's not really reasonable for make check though. Why not? When I was working on GCC I usually ran a subset of the testsuite manually and then did a full run overnight. I said <4 hours because it lets you do 2 runs (baseline and patched) while you sleep. However I agree it's more than we're used to, so I'd not put it under "make check". Still, having it available from make would be nice. >> and is more or less guaranteed to pass. > > That's still the main challenge. Yep. :( >> qtest could at best host some sanity checks on the ACPI tables, which >> would catch the MCFG problems that Gerd reported on v5. > > Depends on how deep the test understands ACPI - the signature > was wrong I think. > > Note I was testing this too - comparing tables between > revisions. I just didn't notice that list of tables > to test included was generated by me on piix, so > MCFG wasn't tested. So we could have a qtest for sanity checking ACPI tables. At least fw_cfg is one of the few components that has qtest infrastructure... I don't think we need to do more than that though. The set of sanity checks can start with a simple list of tables that "have to be there" for a given machine type. Paolo
On Wed, Oct 16, 2013 at 3:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 17/10/2013 00:03, Michael S. Tsirkin ha scritto: >> On Wed, Oct 16, 2013 at 11:26:11PM +0200, Paolo Bonzini wrote: >>> Il 16/10/2013 20:37, Michael S. Tsirkin ha scritto: >>>> Gleb, Paolo, what do you think? OK to merge kvm unit test >>>> into qemu? It depends on qemu anyway, in-tree will make it easier. >>>> Maybe someone's looking at this already? >>> >>> I think merging KVM unit tests doesn't make much sense because, with >>> some small exceptions, it is mostly a test or a benchmark for KVM. >> >> But why keep them separate? They need qemu to work, don't they? > > Not necessarily. They need a userspace component of course, but most of > them do not need something as big as QEMU. Most tests, perhaps all, > only write to a handful of ports and use no BIOS services. > >>> What >>> may make sense is to have a quick way to run autotest on a QEMU tree, >>> with a subset of testcases that doesn't take too much time (let's say <4 >>> hours) >> >> That's not really reasonable for make check though. > > Why not? When I was working on GCC I usually ran a subset of the > testsuite manually and then did a full run overnight. I said <4 hours > because it lets you do 2 runs (baseline and patched) while you sleep. > > However I agree it's more than we're used to, so I'd not put it under > "make check". Still, having it available from make would be nice. > >>> and is more or less guaranteed to pass. >> >> That's still the main challenge. > > Yep. :( > >>> qtest could at best host some sanity checks on the ACPI tables, which >>> would catch the MCFG problems that Gerd reported on v5. >> >> Depends on how deep the test understands ACPI - the signature >> was wrong I think. >> >> Note I was testing this too - comparing tables between >> revisions. I just didn't notice that list of tables >> to test included was generated by me on piix, so >> MCFG wasn't tested. > > So we could have a qtest for sanity checking ACPI tables. At least > fw_cfg is one of the few components that has qtest infrastructure... I > don't think we need to do more than that though. The set of sanity > checks can start with a simple list of tables that "have to be there" > for a given machine type. I think we could reasonably attempt to validate ACPI tables across machine versions. Since this is qtest, we can even do things like use iasl to disassemble the blobs on the host. This could be pretty handy for detecting compatibility issues across machine versions. Regards, Anthony Liguori > > Paolo
On Wed, Oct 16, 2013 at 04:52:35PM -0700, Anthony Liguori wrote: > On Wed, Oct 16, 2013 at 3:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il 17/10/2013 00:03, Michael S. Tsirkin ha scritto: > >> On Wed, Oct 16, 2013 at 11:26:11PM +0200, Paolo Bonzini wrote: > >>> Il 16/10/2013 20:37, Michael S. Tsirkin ha scritto: > >>>> Gleb, Paolo, what do you think? OK to merge kvm unit test > >>>> into qemu? It depends on qemu anyway, in-tree will make it easier. > >>>> Maybe someone's looking at this already? > >>> > >>> I think merging KVM unit tests doesn't make much sense because, with > >>> some small exceptions, it is mostly a test or a benchmark for KVM. > >> > >> But why keep them separate? They need qemu to work, don't they? > > > > Not necessarily. They need a userspace component of course, but most of > > them do not need something as big as QEMU. Most tests, perhaps all, > > only write to a handful of ports and use no BIOS services. > > > >>> What > >>> may make sense is to have a quick way to run autotest on a QEMU tree, > >>> with a subset of testcases that doesn't take too much time (let's say <4 > >>> hours) > >> > >> That's not really reasonable for make check though. > > > > Why not? When I was working on GCC I usually ran a subset of the > > testsuite manually and then did a full run overnight. I said <4 hours > > because it lets you do 2 runs (baseline and patched) while you sleep. > > > > However I agree it's more than we're used to, so I'd not put it under > > "make check". Still, having it available from make would be nice. > > > >>> and is more or less guaranteed to pass. > >> > >> That's still the main challenge. > > > > Yep. :( > > > >>> qtest could at best host some sanity checks on the ACPI tables, which > >>> would catch the MCFG problems that Gerd reported on v5. > >> > >> Depends on how deep the test understands ACPI - the signature > >> was wrong I think. > >> > >> Note I was testing this too - comparing tables between > >> revisions. I just didn't notice that list of tables > >> to test included was generated by me on piix, so > >> MCFG wasn't tested. > > > > So we could have a qtest for sanity checking ACPI tables. At least > > fw_cfg is one of the few components that has qtest infrastructure... I > > don't think we need to do more than that though. The set of sanity > > checks can start with a simple list of tables that "have to be there" > > for a given machine type. > > I think we could reasonably attempt to validate ACPI tables across > machine versions. > > Since this is qtest, we can even do things like use iasl to > disassemble the blobs on the host. This could be pretty handy for > detecting compatibility issues across machine versions. > > Regards, > > Anthony Liguori Sounds nifty - comparing dis-assembled output is exactly how I tested this manually. This solves the problem that ACPI allows many ways to encode identical tables. > > > > Paolo
On Thu, Oct 17, 2013 at 12:25:32AM +0200, Paolo Bonzini wrote: > Il 17/10/2013 00:03, Michael S. Tsirkin ha scritto: > > On Wed, Oct 16, 2013 at 11:26:11PM +0200, Paolo Bonzini wrote: > >> Il 16/10/2013 20:37, Michael S. Tsirkin ha scritto: > >>> Gleb, Paolo, what do you think? OK to merge kvm unit test > >>> into qemu? It depends on qemu anyway, in-tree will make it easier. > >>> Maybe someone's looking at this already? > >> > >> I think merging KVM unit tests doesn't make much sense because, with > >> some small exceptions, it is mostly a test or a benchmark for KVM. > > > > But why keep them separate? They need qemu to work, don't they? > > Not necessarily. They need a userspace component of course, but most of > them do not need something as big as QEMU. Most tests, perhaps all, > only write to a handful of ports and use no BIOS services. Well all of them use the test device to report status, right? Do they work on e.g. kvmtool? If anyone uses these tests outside QEMU then maybe it's worth it to be nice and keep it separate. If not it's just extra pain and compatibility worries without any real gain - and if someone starts using it this way down the line we'll be able to use something like git subtree to extract it again. For example I think it might be nice to switch everyone to use pci device instead of fixed ports - less magic numbers this way. But if I need to keep it working on old qemu and keep two versions of code around, I'd probably not bother. > >> What > >> may make sense is to have a quick way to run autotest on a QEMU tree, > >> with a subset of testcases that doesn't take too much time (let's say <4 > >> hours) > > > > That's not really reasonable for make check though. > > Why not? When I was working on GCC I usually ran a subset of the > testsuite manually and then did a full run overnight. I said <4 hours > because it lets you do 2 runs (baseline and patched) while you sleep. > > However I agree it's more than we're used to, so I'd not put it under > "make check". Still, having it available from make would be nice. > > >> and is more or less guaranteed to pass. > > > > That's still the main challenge. > > Yep. :( > > >> qtest could at best host some sanity checks on the ACPI tables, which > >> would catch the MCFG problems that Gerd reported on v5. > > > > Depends on how deep the test understands ACPI - the signature > > was wrong I think. > > > > Note I was testing this too - comparing tables between > > revisions. I just didn't notice that list of tables > > to test included was generated by me on piix, so > > MCFG wasn't tested. > > So we could have a qtest for sanity checking ACPI tables. At least > fw_cfg is one of the few components that has qtest infrastructure... I > don't think we need to do more than that though. The set of sanity > checks can start with a simple list of tables that "have to be there" > for a given machine type. > > Paolo
On Thu, Oct 17, 2013 at 12:25:32AM +0200, Paolo Bonzini wrote: > >> qtest could at best host some sanity checks on the ACPI tables, which > >> would catch the MCFG problems that Gerd reported on v5. > > > > Depends on how deep the test understands ACPI - the signature > > was wrong I think. > > > > Note I was testing this too - comparing tables between > > revisions. I just didn't notice that list of tables > > to test included was generated by me on piix, so > > MCFG wasn't tested. > > So we could have a qtest for sanity checking ACPI tables. At least > fw_cfg is one of the few components that has qtest infrastructure... I > don't think we need to do more than that though. The set of sanity > checks can start with a simple list of tables that "have to be there" > for a given machine type. > > Paolo Well this means these tests won't pass on old qemu so they would be useless for comparing old to new. And in this case, while it's a good idea to have them I don't see why it's a blocker for merging.
On Thu, Oct 17, 2013 at 08:32:14AM +0300, Michael S. Tsirkin wrote: > On Thu, Oct 17, 2013 at 12:25:32AM +0200, Paolo Bonzini wrote: > > Il 17/10/2013 00:03, Michael S. Tsirkin ha scritto: > > > On Wed, Oct 16, 2013 at 11:26:11PM +0200, Paolo Bonzini wrote: > > >> Il 16/10/2013 20:37, Michael S. Tsirkin ha scritto: > > >>> Gleb, Paolo, what do you think? OK to merge kvm unit test > > >>> into qemu? It depends on qemu anyway, in-tree will make it easier. > > >>> Maybe someone's looking at this already? > > >> > > >> I think merging KVM unit tests doesn't make much sense because, with > > >> some small exceptions, it is mostly a test or a benchmark for KVM. > > > > > > But why keep them separate? They need qemu to work, don't they? > > > > Not necessarily. They need a userspace component of course, but most of > > them do not need something as big as QEMU. Most tests, perhaps all, > > only write to a handful of ports and use no BIOS services. > > Well all of them use the test device to report status, right? > Do they work on e.g. kvmtool? > If anyone uses these tests outside QEMU then maybe it's > worth it to be nice and keep it separate. > If not it's just extra pain and compatibility worries without > any real gain - and if someone starts using it > this way down the line we'll be able to use something > like git subtree to extract it again. > Same logic can be used to argue that unittest should be part of Linux tree. Unittest was part of qemu-kvm and it was decided that having separate repository for it is much better. Time showed that this is true. Look at unittests commits and see how much of them have something to do with QEMU changes and how much do not, and now look at the same commit and see how much of them has something to do with kernel. See? The argument for putting unittest into kernel repo is much stronger. > For example I think it might be nice to switch everyone > to use pci device instead of fixed ports - less magic > numbers this way. But if I need to keep it working on > old qemu and keep two versions of code around, I'd > probably not bother. > Nothing nice about it. Test device is all about simplicity. If separate repository stops one from even attempting it the separation was already a win :) -- Gleb.
Hi, > By far the best way to test this is to boot some guest, > download tables, then run iasl -d on them. Fully agree. /me did the same when testing with coreboot. Boot linux, with old seabios, with new seabios, with (patched) coreboot, then diffed the iasl decompiled tables. cheers, Gerd
Il 17/10/2013 07:34, Michael S. Tsirkin ha scritto: > > So we could have a qtest for sanity checking ACPI tables. At least > > fw_cfg is one of the few components that has qtest infrastructure... I > > don't think we need to do more than that though. The set of sanity > > checks can start with a simple list of tables that "have to be there" > > for a given machine type. > > Well this means these tests won't pass on old qemu so > they would be useless for comparing old to new. Yes, they would be correctness tests not compatibility tests. > And in this case, while it's a good idea to have them > I don't see why it's a blocker for merging. In principle, correctness tests should be a prerequisite for merging something. Would you merge RCU without an equivalent of rcutorture? We rarely if ever obey that principle, but we sometimes do (e.g. QAPI has pretty good test cases, and new additions to util/ almost always get new testcases these days). Paolo
On Mon, 14 Oct 2013 18:01:20 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > Add support for acpi pci hotplug using the > new infrastructure. > PIIX4 legacy interface is maintained as is for > machine types 1.6 and older. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/hw/i386/pc.h | 5 ++++ > hw/acpi/piix4.c | 75 +++++++++++++++++++++++++++++++++++++++++++++------- > 2 files changed, 70 insertions(+), 10 deletions(-) > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 39db8cb..6865972 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -249,6 +249,11 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t); > > #define PC_COMPAT_1_6 \ > {\ > + .driver = "PIIX4_PM",\ > + .property = "acpi-pci-hotplug-with-bridge-support",\ > + .value = "off",\ > + }, \ > + {\ > .driver = "e1000",\ > .property = "mitigation",\ > .value = "off",\ > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index 3bcd890..d516033 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -30,6 +30,7 @@ > #include "hw/nvram/fw_cfg.h" > #include "exec/address-spaces.h" > #include "hw/acpi/piix4.h" > +#include "hw/acpi/pcihp.h" > > //#define DEBUG > > @@ -73,7 +74,6 @@ typedef struct PIIX4PMState { > uint32_t io_base; > > MemoryRegion io_gpe; > - MemoryRegion io_pci; > MemoryRegion io_cpu; > ACPIREGS ar; > > @@ -88,11 +88,16 @@ typedef struct PIIX4PMState { > Notifier machine_ready; > Notifier powerdown_notifier; > > - /* for pci hotplug */ > + /* for legacy pci hotplug (compatible with qemu 1.6 and older) */ > + MemoryRegion io_pci; > struct pci_status pci0_status; > uint32_t pci0_hotplug_enable; > uint32_t pci0_slot_device_present; > > + /* for new pci hotplug (with PCI2PCI bridge support) */ > + AcpiPciHpState acpi_pci_hotplug; pcihp therm collides with linux's pcihp driver that is used with native PCIE hotplug, which makes code a bit confusing at first. perhaps replacing "pcihp" through out with more verbose "pci_hotplug" would be better? > + bool use_acpi_pci_hotplug; > + > uint8_t disable_s3; > uint8_t disable_s4; > uint8_t s4_val; > @@ -282,6 +287,18 @@ static int acpi_load_old(QEMUFile *f, void *opaque, int version_id) > return ret; > } > > +static bool vmstate_test_use_acpi_pci_hotplug(void *opaque, int version_id) > +{ > + PIIX4PMState *s = opaque; > + return s->use_acpi_pci_hotplug; > +} > + > +static bool vmstate_test_no_use_acpi_pci_hotplug(void *opaque, int version_id) > +{ > + PIIX4PMState *s = opaque; > + return !s->use_acpi_pci_hotplug; > +} > + > /* qemu-kvm 1.2 uses version 3 but advertised as 2 > * To support incoming qemu-kvm 1.2 migration, change version_id > * and minimum_version_id to 2 below (which breaks migration from > @@ -304,8 +321,12 @@ static const VMStateDescription vmstate_acpi = { > VMSTATE_TIMER(ar.tmr.timer, PIIX4PMState), > VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState), > VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE), > - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, > - struct pci_status), > + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, > + vmstate_test_no_use_acpi_pci_hotplug, > + 2, vmstate_pci_status, > + struct pci_status), > + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState, > + vmstate_test_use_acpi_pci_hotplug), > VMSTATE_END_OF_LIST() > } > }; > @@ -383,7 +404,11 @@ static void piix4_reset(void *opaque) > pci_conf[0x5B] = 0x02; > } > pm_io_space_update(s); > - piix4_update_hotplug(s); > + if (s->use_acpi_pci_hotplug) { > + acpi_pcihp_reset(&s->acpi_pci_hotplug); > + } else { > + piix4_update_hotplug(s); > + } > } > > static void piix4_pm_powerdown_req(Notifier *n, void *opaque) > @@ -394,6 +419,26 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque) > acpi_pm1_evt_power_down(&s->ar); > } > > +static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev, > + PCIHotplugState state) > +{ > + PIIX4PMState *s = PIIX4_PM(qdev); > + int ret = acpi_pcihp_device_hotplug(&s->acpi_pci_hotplug, dev, state); > + if (ret < 0) { > + return ret; > + } > + s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > + > + pm_update_sci(s); > + return 0; > +} > + > +static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque) > +{ > + PIIX4PMState *s = opaque; > + pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s)); > +} > + > static void piix4_pm_machine_ready(Notifier *n, void *opaque) > { > PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready); > @@ -407,6 +452,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque) > pci_conf[0x63] = 0x60; > pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) | > (memory_region_present(io_as, 0x2f8) ? 0x90 : 0); > + > + if (s->use_acpi_pci_hotplug) { > + pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s); why do you need to do it dinamically? > + } > } > > static void piix4_pm_add_propeties(PIIX4PMState *s) > @@ -528,6 +577,8 @@ static Property piix4_pm_properties[] = { > DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0), > DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0), > DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2), > + DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState, > + use_acpi_pci_hotplug, true), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -716,11 +767,15 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > "acpi-gpe0", GPE_LEN); > memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); > > - memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s, > - "acpi-pci-hotplug", PCI_HOTPLUG_SIZE); > - memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR, > - &s->io_pci); > - pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s)); > + if (s->use_acpi_pci_hotplug) { > + acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent); > + } else { > + memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s, > + "acpi-pci-hotplug", PCI_HOTPLUG_SIZE); > + memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR, > + &s->io_pci); > + pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s)); > + } > > CPU_FOREACH(cpu) { > CPUClass *cc = CPU_GET_CLASS(cpu); > -- > MST > >
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 39db8cb..6865972 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -249,6 +249,11 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t); #define PC_COMPAT_1_6 \ {\ + .driver = "PIIX4_PM",\ + .property = "acpi-pci-hotplug-with-bridge-support",\ + .value = "off",\ + }, \ + {\ .driver = "e1000",\ .property = "mitigation",\ .value = "off",\ diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 3bcd890..d516033 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -30,6 +30,7 @@ #include "hw/nvram/fw_cfg.h" #include "exec/address-spaces.h" #include "hw/acpi/piix4.h" +#include "hw/acpi/pcihp.h" //#define DEBUG @@ -73,7 +74,6 @@ typedef struct PIIX4PMState { uint32_t io_base; MemoryRegion io_gpe; - MemoryRegion io_pci; MemoryRegion io_cpu; ACPIREGS ar; @@ -88,11 +88,16 @@ typedef struct PIIX4PMState { Notifier machine_ready; Notifier powerdown_notifier; - /* for pci hotplug */ + /* for legacy pci hotplug (compatible with qemu 1.6 and older) */ + MemoryRegion io_pci; struct pci_status pci0_status; uint32_t pci0_hotplug_enable; uint32_t pci0_slot_device_present; + /* for new pci hotplug (with PCI2PCI bridge support) */ + AcpiPciHpState acpi_pci_hotplug; + bool use_acpi_pci_hotplug; + uint8_t disable_s3; uint8_t disable_s4; uint8_t s4_val; @@ -282,6 +287,18 @@ static int acpi_load_old(QEMUFile *f, void *opaque, int version_id) return ret; } +static bool vmstate_test_use_acpi_pci_hotplug(void *opaque, int version_id) +{ + PIIX4PMState *s = opaque; + return s->use_acpi_pci_hotplug; +} + +static bool vmstate_test_no_use_acpi_pci_hotplug(void *opaque, int version_id) +{ + PIIX4PMState *s = opaque; + return !s->use_acpi_pci_hotplug; +} + /* qemu-kvm 1.2 uses version 3 but advertised as 2 * To support incoming qemu-kvm 1.2 migration, change version_id * and minimum_version_id to 2 below (which breaks migration from @@ -304,8 +321,12 @@ static const VMStateDescription vmstate_acpi = { VMSTATE_TIMER(ar.tmr.timer, PIIX4PMState), VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState), VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE), - VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status, - struct pci_status), + VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState, + vmstate_test_no_use_acpi_pci_hotplug, + 2, vmstate_pci_status, + struct pci_status), + VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState, + vmstate_test_use_acpi_pci_hotplug), VMSTATE_END_OF_LIST() } }; @@ -383,7 +404,11 @@ static void piix4_reset(void *opaque) pci_conf[0x5B] = 0x02; } pm_io_space_update(s); - piix4_update_hotplug(s); + if (s->use_acpi_pci_hotplug) { + acpi_pcihp_reset(&s->acpi_pci_hotplug); + } else { + piix4_update_hotplug(s); + } } static void piix4_pm_powerdown_req(Notifier *n, void *opaque) @@ -394,6 +419,26 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque) acpi_pm1_evt_power_down(&s->ar); } +static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev, + PCIHotplugState state) +{ + PIIX4PMState *s = PIIX4_PM(qdev); + int ret = acpi_pcihp_device_hotplug(&s->acpi_pci_hotplug, dev, state); + if (ret < 0) { + return ret; + } + s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; + + pm_update_sci(s); + return 0; +} + +static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque) +{ + PIIX4PMState *s = opaque; + pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s)); +} + static void piix4_pm_machine_ready(Notifier *n, void *opaque) { PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready); @@ -407,6 +452,10 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque) pci_conf[0x63] = 0x60; pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) | (memory_region_present(io_as, 0x2f8) ? 0x90 : 0); + + if (s->use_acpi_pci_hotplug) { + pci_for_each_bus(d->bus, piix4_update_bus_hotplug, s); + } } static void piix4_pm_add_propeties(PIIX4PMState *s) @@ -528,6 +577,8 @@ static Property piix4_pm_properties[] = { DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0), DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0), DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2), + DEFINE_PROP_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState, + use_acpi_pci_hotplug, true), DEFINE_PROP_END_OF_LIST(), }; @@ -716,11 +767,15 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, "acpi-gpe0", GPE_LEN); memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe); - memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s, - "acpi-pci-hotplug", PCI_HOTPLUG_SIZE); - memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR, - &s->io_pci); - pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s)); + if (s->use_acpi_pci_hotplug) { + acpi_pcihp_init(&s->acpi_pci_hotplug, bus, parent); + } else { + memory_region_init_io(&s->io_pci, OBJECT(s), &piix4_pci_ops, s, + "acpi-pci-hotplug", PCI_HOTPLUG_SIZE); + memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR, + &s->io_pci); + pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s)); + } CPU_FOREACH(cpu) { CPUClass *cc = CPU_GET_CLASS(cpu);
Add support for acpi pci hotplug using the new infrastructure. PIIX4 legacy interface is maintained as is for machine types 1.6 and older. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- include/hw/i386/pc.h | 5 ++++ hw/acpi/piix4.c | 75 +++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 70 insertions(+), 10 deletions(-)