Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Wed, 14 Oct 2015 13:47:32 -0300
Eduardo Habkost wrote:
> On Wed, Oct 14, 2015 at 10:45:00AM +0200, Igor Mammedov wrote:
> > On Tue, 13 Oct 2015 16:10:03 -0300
> > Eduardo Habkost wrote:
> >
> > > On Sat, Oct 10, 2015 at 12:00:16AM -0400, Gabriel L. Somlo wrote:
> > > > On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
> [...]
> > > > > > >> +if (!pcmc->acpi_no_fw_cfg_node) {
> > > > > > > we don't really care about SSDT size changes since during
> > > > > > > migration ACPI blobs will be migrated from source, so
> > > > > > > machine compat bloat is excessive here. It would be better
> > > > > > > to just remove it.
> > >
> > > What about non-live migration?
> > I don't see any issues here, it should just work, since usually
> > original SSDT from source is used (copied as part of migrated ram).
>
> I mean: shutdown, followed by QEMU upgrade, followed by reboot.
Then guest will get new ACPI tables as well possibly updated BIOS
regardless of machine type. (see below)
>
> > > > > >
> > > > > > This was Eduardo's suggestion, if I recall correctly:
> > > > > >
> > > > > > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> > > > > >
> > > > > > The idea being, if you move a guest from older QEMU to newer QEMU,
> > > > > > but
> > > > > > keep the machine type (or more precisely, the full machine config,
> > > > > > like
> > > > > > the domain XML) intact, the ACPI device node should not appear out
> > > > > > of
> > > > > > the blue.
> > > > > This ACPI device is an always used resource declaration regardless
> > > > > of machine type so it makes sense to tell guest about used resource.
> > > > >
> > > > > The only reason for machine compat code would be if guest suddenly
> > > > > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > > > > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > > > > as much as possible.
> > > >
> > > > Eduardo, what do you think about this ? I'm hoping to do a v5 over the
> > > > weekend or early next week, and which way this should go is one of the
> > > > couple of decisions that I still have open.
> > >
> > > The general rule is that anything that's visible to the guest shouldn't
> > > change on a QEMU upgrade if the machine-type is kept the same. If we
> > > want to avoid the compat code, we need careful testing to ensure this
> > > won't make any guest OS do something unexpected.
> > >
> > > One of the things that may break if guest-visible bits of the machine
> > > change is Windows license activation, but the rules Windows use to
> > > trigger reactivation aren't very clear.
> > Practice shows that changing ACPI tables doesn't affect MS Activation so
> > far.
>
> I still see any guest-visible change in a machine-type as a bug. But at
> least it seems to be a minor bug.
>
> I just noticed we have made ACPI table changes in the past without any
> compat code, so that's not something new. So I won't mind too much if
> you really want to avoid acpi_no_fw_cfg_node.
That's because ACPI tables are considered to be part (they are in QEMU for
convenience purpose only) of firmware and as expected if one run's
the same machine type with updated BIOS, one'll get updated tables.
So I prefer to avoid adding compat cruft in ACPI code unless
we have to it, which is not case with this patch.
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Wed, Oct 14, 2015 at 10:45:00AM +0200, Igor Mammedov wrote:
> On Tue, 13 Oct 2015 16:10:03 -0300
> Eduardo Habkost wrote:
>
> > On Sat, Oct 10, 2015 at 12:00:16AM -0400, Gabriel L. Somlo wrote:
> > > On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
[...]
> > > > > >> +if (!pcmc->acpi_no_fw_cfg_node) {
> > > > > > we don't really care about SSDT size changes since during
> > > > > > migration ACPI blobs will be migrated from source, so
> > > > > > machine compat bloat is excessive here. It would be better
> > > > > > to just remove it.
> >
> > What about non-live migration?
> I don't see any issues here, it should just work, since usually
> original SSDT from source is used (copied as part of migrated ram).
I mean: shutdown, followed by QEMU upgrade, followed by reboot.
> > > > >
> > > > > This was Eduardo's suggestion, if I recall correctly:
> > > > >
> > > > > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> > > > >
> > > > > The idea being, if you move a guest from older QEMU to newer QEMU, but
> > > > > keep the machine type (or more precisely, the full machine config,
> > > > > like
> > > > > the domain XML) intact, the ACPI device node should not appear out of
> > > > > the blue.
> > > > This ACPI device is an always used resource declaration regardless
> > > > of machine type so it makes sense to tell guest about used resource.
> > > >
> > > > The only reason for machine compat code would be if guest suddenly
> > > > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > > > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > > > as much as possible.
> > >
> > > Eduardo, what do you think about this ? I'm hoping to do a v5 over the
> > > weekend or early next week, and which way this should go is one of the
> > > couple of decisions that I still have open.
> >
> > The general rule is that anything that's visible to the guest shouldn't
> > change on a QEMU upgrade if the machine-type is kept the same. If we
> > want to avoid the compat code, we need careful testing to ensure this
> > won't make any guest OS do something unexpected.
> >
> > One of the things that may break if guest-visible bits of the machine
> > change is Windows license activation, but the rules Windows use to
> > trigger reactivation aren't very clear.
> Practice shows that changing ACPI tables doesn't affect MS Activation so far.
I still see any guest-visible change in a machine-type as a bug. But at
least it seems to be a minor bug.
I just noticed we have made ACPI table changes in the past without any
compat code, so that's not something new. So I won't mind too much if
you really want to avoid acpi_no_fw_cfg_node.
--
Eduardo
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Wed, Oct 14, 2015 at 08:06:36AM +0300, Michael S. Tsirkin wrote: > On Tue, Oct 13, 2015 at 07:43:00PM -0300, Eduardo Habkost wrote: > > On Wed, Oct 14, 2015 at 12:18:10AM +0300, Michael S. Tsirkin wrote: > > > On Tue, Oct 13, 2015 at 04:10:03PM -0300, Eduardo Habkost wrote: > > > > One of the things that may break if guest-visible bits of the machine > > > > change is Windows license activation, but the rules Windows use to > > > > trigger reactivation aren't very clear. > > > > > > They are easy to find on the internet. > > > > I couldn't find them[1]. If you have a pointer to a clear description of > > the rules for all Windows versions, I would love to see it. > > > The detailed info is not hard to find: > http://en.wikipedia.org/wiki/Microsoft_Product_Activation > links to: > http://technet.microsoft.com/en-us/library/bb457054.aspx Thanks. Do you know if there's public information for other Windows versions, or only Windows XP? -- Eduardo
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Tue, 13 Oct 2015 16:10:03 -0300
Eduardo Habkost wrote:
> On Sat, Oct 10, 2015 at 12:00:16AM -0400, Gabriel L. Somlo wrote:
> > On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
> > > On Thu, 1 Oct 2015 10:27:15 +0200
> > > Laszlo Ersek wrote:
> > >
> > > > On 10/01/15 09:02, Igor Mammedov wrote:
> > > > > On Sun, 27 Sep 2015 17:29:00 -0400
> > > > > "Gabriel L. Somlo" wrote:
> > > > >
> > > > >> Add a fw_cfg device node to the ACPI SSDT, on machine types
> > > > >> pc-*-2.5 and up. While the guest-side BIOS can't utilize
> > > > >> this information (since it has to access the hard-coded
> > > > >> fw_cfg device to extract ACPI tables to begin with), having
> > > > >> fw_cfg listed in ACPI will help the guest kernel keep a more
> > > > >> accurate inventory of in-use IO port regions.
> > > > >>
> > > > >> Signed-off-by: Gabriel Somlo
> > > > >> ---
> > > > >> hw/i386/acpi-build.c | 23 +++
> > > > >> hw/i386/pc_piix.c| 1 +
> > > > >> hw/i386/pc_q35.c | 1 +
> > > > >> include/hw/i386/pc.h | 1 +
> > > > >> 4 files changed, 26 insertions(+)
> > > > >>
> > > > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > >> index 95e0c65..ece2710 100644
> > > > >> --- a/hw/i386/acpi-build.c
> > > > >> +++ b/hw/i386/acpi-build.c
> > > > >> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > > >> PcPciInfo *pci, PcGuestInfo *guest_info)
> > > > >> {
> > > > >> MachineState *machine = MACHINE(qdev_get_machine());
> > > > >> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > > > >> uint32_t nr_mem = machine->ram_slots;
> > > > >> unsigned acpi_cpus = guest_info->apic_id_limit;
> > > > >> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs,
> > > > >> *field, *ifctx;
> > > > >> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > > >> aml_append(scope, aml_name_decl("_S5", pkg));
> > > > >> aml_append(ssdt, scope);
> > > > >>
> > > > >> +if (!pcmc->acpi_no_fw_cfg_node) {
> > > > > we don't really care about SSDT size changes since during
> > > > > migration ACPI blobs will be migrated from source, so
> > > > > machine compat bloat is excessive here. It would be better
> > > > > to just remove it.
>
> What about non-live migration?
I don't see any issues here, it should just work, since usually
original SSDT from source is used (copied as part of migrated ram).
> > > >
> > > > This was Eduardo's suggestion, if I recall correctly:
> > > >
> > > > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> > > >
> > > > The idea being, if you move a guest from older QEMU to newer QEMU, but
> > > > keep the machine type (or more precisely, the full machine config, like
> > > > the domain XML) intact, the ACPI device node should not appear out of
> > > > the blue.
> > > This ACPI device is an always used resource declaration regardless
> > > of machine type so it makes sense to tell guest about used resource.
> > >
> > > The only reason for machine compat code would be if guest suddenly
> > > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > > as much as possible.
> >
> > Eduardo, what do you think about this ? I'm hoping to do a v5 over the
> > weekend or early next week, and which way this should go is one of the
> > couple of decisions that I still have open.
>
> The general rule is that anything that's visible to the guest shouldn't
> change on a QEMU upgrade if the machine-type is kept the same. If we
> want to avoid the compat code, we need careful testing to ensure this
> won't make any guest OS do something unexpected.
>
> One of the things that may break if guest-visible bits of the machine
> change is Windows license activation, but the rules Windows use to
> trigger reactivation aren't very clear.
Practice shows that changing ACPI tables doesn't affect MS Activation so far.
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Tue, Oct 13, 2015 at 07:43:00PM -0300, Eduardo Habkost wrote: > On Wed, Oct 14, 2015 at 12:18:10AM +0300, Michael S. Tsirkin wrote: > > On Tue, Oct 13, 2015 at 04:10:03PM -0300, Eduardo Habkost wrote: > > > One of the things that may break if guest-visible bits of the machine > > > change is Windows license activation, but the rules Windows use to > > > trigger reactivation aren't very clear. > > > > They are easy to find on the internet. > > I couldn't find them[1]. If you have a pointer to a clear description of > the rules for all Windows versions, I would love to see it. The detailed info is not hard to find: http://en.wikipedia.org/wiki/Microsoft_Product_Activation links to: http://technet.microsoft.com/en-us/library/bb457054.aspx 1 Display Adapter 00010 (5) 2 SCSI Adapter 00011 (5) 3 IDE Adapter 0011 (4) 4 Network Adapter MAC Address 1001011000 (10) 5 RAM Amount Range (i.e. 0-64mb, 64-128mb, etc) 101 (3) 6 Processor Type 011 (3) 7 Processor Serial Number 00 (6) 8 Hard Drive Device 1101100 (7) 9 Hard Drive Volume Serial Number 100101 (10) 10 CD—ROM / CD-RW / DVD-ROM 010111 (6) - "Dockable" 0 (1) - Hardware Hash version (version of algorithm used) 001 (3) Also: Microsoft defines "substantially different" hardware differently for PCs that are configured to be dockable. Additionally, the network adapter is given a superior "weighting." If the PC is not dockable and a network adapter exists and is not changed, 6 or more of the other above values would have to change before reactivation was required. If a network adapter existed but is changed or never existed at all, 4 or more changes (including the changed network adapter if it previously existed) will result in a requirement to reactivate. So it's also not hard to try it out with kvm. Apply a patch, change 3 other things: nic mac, ram size (by a factor of >2), cpu type and boot. > > > >___ > >Xen-devel mailing list > >xen-de...@lists.xen.org > >http://lists.xen.org/xen-devel > > > >- > >No virus found in this message. > >Checked by AVG - www.avg.com > >Version: 2014.0.4592 / Virus Database: 3986/7769 - Release Date: 06/30/14 > > > > > -- > Ross Philipson
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Wed, Oct 14, 2015 at 12:18:10AM +0300, Michael S. Tsirkin wrote: > On Tue, Oct 13, 2015 at 04:10:03PM -0300, Eduardo Habkost wrote: > > One of the things that may break if guest-visible bits of the machine > > change is Windows license activation, but the rules Windows use to > > trigger reactivation aren't very clear. > > They are easy to find on the internet. I couldn't find them[1]. If you have a pointer to a clear description of the rules for all Windows versions, I would love to see it. [1] All I have found were things like: "Do I need to activate Windows after making a hardware change? Maybe. [...]" http://windows.microsoft.com/en-us/windows/activating-windows-faq#1TC=windows-7 "Microsoft is characteristically shy about discussing the details of activation. [...]" http://www.zdnet.com/article/microsoft-quietly-rewrites-its-activation-rules-for-windows-10/ "What triggers the need to reactivate Windows? As intended, each hardware component gets a relative weight, and from that WGA determines whether your copy of Windows 7 needs reactivation. The weight and the number of changes is apparently a guarded secret. If you upgrade too much at once, WAT decides that your PC is new, and things can get messy. The actual algorithm that Microsoft uses is not disclosed, [...]" http://searchitchannel.techtarget.com/feature/How-Windows-7-hardware-upgrades-affect-licensing -- Eduardo
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Tue, Oct 13, 2015 at 04:10:03PM -0300, Eduardo Habkost wrote: > One of the things that may break if guest-visible bits of the machine > change is Windows license activation, but the rules Windows use to > trigger reactivation aren't very clear. They are easy to find on the internet. > -- > Eduardo
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Sat, Oct 10, 2015 at 12:00:16AM -0400, Gabriel L. Somlo wrote:
> On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
> > On Thu, 1 Oct 2015 10:27:15 +0200
> > Laszlo Ersek wrote:
> >
> > > On 10/01/15 09:02, Igor Mammedov wrote:
> > > > On Sun, 27 Sep 2015 17:29:00 -0400
> > > > "Gabriel L. Somlo" wrote:
> > > >
> > > >> Add a fw_cfg device node to the ACPI SSDT, on machine types
> > > >> pc-*-2.5 and up. While the guest-side BIOS can't utilize
> > > >> this information (since it has to access the hard-coded
> > > >> fw_cfg device to extract ACPI tables to begin with), having
> > > >> fw_cfg listed in ACPI will help the guest kernel keep a more
> > > >> accurate inventory of in-use IO port regions.
> > > >>
> > > >> Signed-off-by: Gabriel Somlo
> > > >> ---
> > > >> hw/i386/acpi-build.c | 23 +++
> > > >> hw/i386/pc_piix.c| 1 +
> > > >> hw/i386/pc_q35.c | 1 +
> > > >> include/hw/i386/pc.h | 1 +
> > > >> 4 files changed, 26 insertions(+)
> > > >>
> > > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > >> index 95e0c65..ece2710 100644
> > > >> --- a/hw/i386/acpi-build.c
> > > >> +++ b/hw/i386/acpi-build.c
> > > >> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >> PcPciInfo *pci, PcGuestInfo *guest_info)
> > > >> {
> > > >> MachineState *machine = MACHINE(qdev_get_machine());
> > > >> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > > >> uint32_t nr_mem = machine->ram_slots;
> > > >> unsigned acpi_cpus = guest_info->apic_id_limit;
> > > >> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
> > > >> *ifctx;
> > > >> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >> aml_append(scope, aml_name_decl("_S5", pkg));
> > > >> aml_append(ssdt, scope);
> > > >>
> > > >> +if (!pcmc->acpi_no_fw_cfg_node) {
> > > > we don't really care about SSDT size changes since during
> > > > migration ACPI blobs will be migrated from source, so
> > > > machine compat bloat is excessive here. It would be better
> > > > to just remove it.
What about non-live migration?
> > >
> > > This was Eduardo's suggestion, if I recall correctly:
> > >
> > > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> > >
> > > The idea being, if you move a guest from older QEMU to newer QEMU, but
> > > keep the machine type (or more precisely, the full machine config, like
> > > the domain XML) intact, the ACPI device node should not appear out of
> > > the blue.
> > This ACPI device is an always used resource declaration regardless
> > of machine type so it makes sense to tell guest about used resource.
> >
> > The only reason for machine compat code would be if guest suddenly
> > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > as much as possible.
>
> Eduardo, what do you think about this ? I'm hoping to do a v5 over the
> weekend or early next week, and which way this should go is one of the
> couple of decisions that I still have open.
The general rule is that anything that's visible to the guest shouldn't
change on a QEMU upgrade if the machine-type is kept the same. If we
want to avoid the compat code, we need careful testing to ensure this
won't make any guest OS do something unexpected.
One of the things that may break if guest-visible bits of the machine
change is Windows license activation, but the rules Windows use to
trigger reactivation aren't very clear.
--
Eduardo
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Thu, Oct 01, 2015 at 01:33:50PM +0200, Igor Mammedov wrote:
> On Thu, 1 Oct 2015 10:27:15 +0200
> Laszlo Ersek wrote:
>
> > On 10/01/15 09:02, Igor Mammedov wrote:
> > > On Sun, 27 Sep 2015 17:29:00 -0400
> > > "Gabriel L. Somlo" wrote:
> > >
> > >> Add a fw_cfg device node to the ACPI SSDT, on machine types
> > >> pc-*-2.5 and up. While the guest-side BIOS can't utilize
> > >> this information (since it has to access the hard-coded
> > >> fw_cfg device to extract ACPI tables to begin with), having
> > >> fw_cfg listed in ACPI will help the guest kernel keep a more
> > >> accurate inventory of in-use IO port regions.
> > >>
> > >> Signed-off-by: Gabriel Somlo
> > >> ---
> > >> hw/i386/acpi-build.c | 23 +++
> > >> hw/i386/pc_piix.c| 1 +
> > >> hw/i386/pc_q35.c | 1 +
> > >> include/hw/i386/pc.h | 1 +
> > >> 4 files changed, 26 insertions(+)
> > >>
> > >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > >> index 95e0c65..ece2710 100644
> > >> --- a/hw/i386/acpi-build.c
> > >> +++ b/hw/i386/acpi-build.c
> > >> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >> PcPciInfo *pci, PcGuestInfo *guest_info)
> > >> {
> > >> MachineState *machine = MACHINE(qdev_get_machine());
> > >> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > >> uint32_t nr_mem = machine->ram_slots;
> > >> unsigned acpi_cpus = guest_info->apic_id_limit;
> > >> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
> > >> *ifctx;
> > >> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >> aml_append(scope, aml_name_decl("_S5", pkg));
> > >> aml_append(ssdt, scope);
> > >>
> > >> +if (!pcmc->acpi_no_fw_cfg_node) {
> > > we don't really care about SSDT size changes since during
> > > migration ACPI blobs will be migrated from source, so
> > > machine compat bloat is excessive here. It would be better
> > > to just remove it.
> >
> > This was Eduardo's suggestion, if I recall correctly:
> >
> > http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> >
> > The idea being, if you move a guest from older QEMU to newer QEMU, but
> > keep the machine type (or more precisely, the full machine config, like
> > the domain XML) intact, the ACPI device node should not appear out of
> > the blue.
> This ACPI device is an always used resource declaration regardless
> of machine type so it makes sense to tell guest about used resource.
>
> The only reason for machine compat code would be if guest suddenly
> started to ask for a driver but as Gabriel showed with _STA(0xB)
> it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> as much as possible.
Eduardo, what do you think about this ? I'm hoping to do a v5 over the
weekend or early next week, and which way this should go is one of the
couple of decisions that I still have open.
Thanks much,
--Gabriel
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On 10/01/2015 05:52 AM, Laszlo Ersek wrote: >> PS: >> is it me alone or email to qemu-devel arrives with huge delay (upto 2 days)? > > Several subscribers have reported the same, and I'm seeing delays myself. gnu.org servers (hosts of our nongnu.org list) got hit with a huge spam bomb last weekend; in the process of dealing with the system overload from that, LOTS of lists had traffic being held for multiple hours. It looks like the mail is a bit snappier today, but still plowing through the backlog of mails that were pending retries when the systems weren't as loaded. I wish there were a nice URL to post to an outage report from the fsfadmin folks, but can't find one. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Thu, Oct 01, 2015 at 01:52:59PM +0200, Laszlo Ersek wrote:
> On 10/01/15 13:33, Igor Mammedov wrote:
> > On Thu, 1 Oct 2015 10:27:15 +0200
> > Laszlo Ersek wrote:
> >
> >> On 10/01/15 09:02, Igor Mammedov wrote:
> >>> On Sun, 27 Sep 2015 17:29:00 -0400
> >>> "Gabriel L. Somlo" wrote:
> >>>
> Add a fw_cfg device node to the ACPI SSDT, on machine types
> pc-*-2.5 and up. While the guest-side BIOS can't utilize
> this information (since it has to access the hard-coded
> fw_cfg device to extract ACPI tables to begin with), having
> fw_cfg listed in ACPI will help the guest kernel keep a more
> accurate inventory of in-use IO port regions.
>
> Signed-off-by: Gabriel Somlo
> ---
> hw/i386/acpi-build.c | 23 +++
> hw/i386/pc_piix.c| 1 +
> hw/i386/pc_q35.c | 1 +
> include/hw/i386/pc.h | 1 +
> 4 files changed, 26 insertions(+)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..ece2710 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> PcPciInfo *pci, PcGuestInfo *guest_info)
> {
> MachineState *machine = MACHINE(qdev_get_machine());
> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> uint32_t nr_mem = machine->ram_slots;
> unsigned acpi_cpus = guest_info->apic_id_limit;
> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
> *ifctx;
> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> aml_append(scope, aml_name_decl("_S5", pkg));
> aml_append(ssdt, scope);
>
> +if (!pcmc->acpi_no_fw_cfg_node) {
> >>> we don't really care about SSDT size changes since during
> >>> migration ACPI blobs will be migrated from source, so
> >>> machine compat bloat is excessive here. It would be better
> >>> to just remove it.
> >>
> >> This was Eduardo's suggestion, if I recall correctly:
> >>
> >> http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
> >>
> >> The idea being, if you move a guest from older QEMU to newer QEMU, but
> >> keep the machine type (or more precisely, the full machine config, like
> >> the domain XML) intact, the ACPI device node should not appear out of
> >> the blue.
> > This ACPI device is an always used resource declaration regardless
> > of machine type so it makes sense to tell guest about used resource.
> >
> > The only reason for machine compat code would be if guest suddenly
> > started to ask for a driver but as Gabriel showed with _STA(0xB)
> > it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> > as much as possible.
>
> Fine by me, but I think Eduardo should agree (or disagree) as well,
> because it was his point originally.
I have no problem taking the compat logic back out, if Eduardo agrees
that's an OK thing to do.
> +scope = aml_scope("\\_SB");
> +dev = aml_device("FWCF");
> +
> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +/* device present, functioning, decoding, not shown in UI */
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +crs = aml_resource_template();
> +/* when using port i/o, the 8-bit data register *always*
> overlaps
> + * with half of the 16-bit control register. Hence, the total
> size
> + * of the i/o region used is FW_CFG_CTL_SIZE */
> +aml_append(crs,
> +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> + 0x01, FW_CFG_CTL_SIZE)
> +);
> >>> could you check/dump this _CRS and PCI0._CRS to see if they are
> >>> intersecting
> >>> in any way?
On bot PIIX and Q35, there's no _CRS under PCI0 in the dsdt.
All I could find was
Scope (\_SB.PCI0)
{
Name (_CRS, ...)
IO (Decode16,
0x0CF8, // Range Minimum
0x0CF8, // Range Maximum
0x01, // Alignment
0x08, // Length
)
...
So that does not overlap with fw_cfg.
In fact, I searched for all "IO (Decode..." occurrences, and nothing
in either DSDT or SSDT has any overlap with [ 0x510 .. 0x512 ].
Thanks,
-- Gabriel
> >>>
> >>>
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +aml_append(scope, dev);
> +aml_append(ssdt, scope);
> +}
> +
> if (misc->applesmc_io_base) {
> scope = aml_scope("\\_SB.PCI0.ISA");
> dev = aml_device("SMC");
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 3ffb05f..7f5e5d9 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -482,6 +482,7 @@ static void
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Thu, 1 Oct 2015 10:27:15 +0200
Laszlo Ersek wrote:
> On 10/01/15 09:02, Igor Mammedov wrote:
> > On Sun, 27 Sep 2015 17:29:00 -0400
> > "Gabriel L. Somlo" wrote:
> >
> >> Add a fw_cfg device node to the ACPI SSDT, on machine types
> >> pc-*-2.5 and up. While the guest-side BIOS can't utilize
> >> this information (since it has to access the hard-coded
> >> fw_cfg device to extract ACPI tables to begin with), having
> >> fw_cfg listed in ACPI will help the guest kernel keep a more
> >> accurate inventory of in-use IO port regions.
> >>
> >> Signed-off-by: Gabriel Somlo
> >> ---
> >> hw/i386/acpi-build.c | 23 +++
> >> hw/i386/pc_piix.c| 1 +
> >> hw/i386/pc_q35.c | 1 +
> >> include/hw/i386/pc.h | 1 +
> >> 4 files changed, 26 insertions(+)
> >>
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index 95e0c65..ece2710 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >> PcPciInfo *pci, PcGuestInfo *guest_info)
> >> {
> >> MachineState *machine = MACHINE(qdev_get_machine());
> >> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> >> uint32_t nr_mem = machine->ram_slots;
> >> unsigned acpi_cpus = guest_info->apic_id_limit;
> >> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
> >> *ifctx;
> >> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> >> aml_append(scope, aml_name_decl("_S5", pkg));
> >> aml_append(ssdt, scope);
> >>
> >> +if (!pcmc->acpi_no_fw_cfg_node) {
> > we don't really care about SSDT size changes since during
> > migration ACPI blobs will be migrated from source, so
> > machine compat bloat is excessive here. It would be better
> > to just remove it.
>
> This was Eduardo's suggestion, if I recall correctly:
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
>
> The idea being, if you move a guest from older QEMU to newer QEMU, but
> keep the machine type (or more precisely, the full machine config, like
> the domain XML) intact, the ACPI device node should not appear out of
> the blue.
This ACPI device is an always used resource declaration regardless
of machine type so it makes sense to tell guest about used resource.
The only reason for machine compat code would be if guest suddenly
started to ask for a driver but as Gabriel showed with _STA(0xB)
it doesn't, so I'm trying to keep ACPI code machine compat agnostic
as much as possible.
PS:
is it me alone or email to qemu-devel arrives with huge delay (upto 2 days)?
>
> I'll let Gabriel answer your other question below. :)
>
> Thanks
> Laszlo
>
>
> >
> >
> >> +scope = aml_scope("\\_SB");
> >> +dev = aml_device("FWCF");
> >> +
> >> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> >> +/* device present, functioning, decoding, not shown in UI */
> >> +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> >> +
> >> +crs = aml_resource_template();
> >> +/* when using port i/o, the 8-bit data register *always* overlaps
> >> + * with half of the 16-bit control register. Hence, the total size
> >> + * of the i/o region used is FW_CFG_CTL_SIZE */
> >> +aml_append(crs,
> >> +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> >> + 0x01, FW_CFG_CTL_SIZE)
> >> +);
> > could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
> > in any way?
> >
> >
> >> +aml_append(dev, aml_name_decl("_CRS", crs));
> >> +
> >> +aml_append(scope, dev);
> >> +aml_append(ssdt, scope);
> >> +}
> >> +
> >> if (misc->applesmc_io_base) {
> >> scope = aml_scope("\\_SB.PCI0.ISA");
> >> dev = aml_device("SMC");
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index 3ffb05f..7f5e5d9 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass
> >> *m)
> >> m->alias = NULL;
> >> m->is_default = 0;
> >> pcmc->broken_reserved_end = true;
> >> +pcmc->acpi_no_fw_cfg_node = true;
> >> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >> }
> >>
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index 1b7d3b6..7180ca3 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> >> pc_q35_2_5_machine_options(m);
> >> m->alias = NULL;
> >> pcmc->broken_reserved_end = true;
> >> +pcmc->acpi_no_fw_cfg_node = true;
> >> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> >> }
> >>
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index 86007e3..6d0f1bd 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -62,6 +62,7 @@ struct PCMachineClass {
> >>
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On 10/01/15 13:33, Igor Mammedov wrote:
> On Thu, 1 Oct 2015 10:27:15 +0200
> Laszlo Ersek wrote:
>
>> On 10/01/15 09:02, Igor Mammedov wrote:
>>> On Sun, 27 Sep 2015 17:29:00 -0400
>>> "Gabriel L. Somlo" wrote:
>>>
Add a fw_cfg device node to the ACPI SSDT, on machine types
pc-*-2.5 and up. While the guest-side BIOS can't utilize
this information (since it has to access the hard-coded
fw_cfg device to extract ACPI tables to begin with), having
fw_cfg listed in ACPI will help the guest kernel keep a more
accurate inventory of in-use IO port regions.
Signed-off-by: Gabriel Somlo
---
hw/i386/acpi-build.c | 23 +++
hw/i386/pc_piix.c| 1 +
hw/i386/pc_q35.c | 1 +
include/hw/i386/pc.h | 1 +
4 files changed, 26 insertions(+)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 95e0c65..ece2710 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
PcPciInfo *pci, PcGuestInfo *guest_info)
{
MachineState *machine = MACHINE(qdev_get_machine());
+PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
uint32_t nr_mem = machine->ram_slots;
unsigned acpi_cpus = guest_info->apic_id_limit;
Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
*ifctx;
@@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
aml_append(scope, aml_name_decl("_S5", pkg));
aml_append(ssdt, scope);
+if (!pcmc->acpi_no_fw_cfg_node) {
>>> we don't really care about SSDT size changes since during
>>> migration ACPI blobs will be migrated from source, so
>>> machine compat bloat is excessive here. It would be better
>>> to just remove it.
>>
>> This was Eduardo's suggestion, if I recall correctly:
>>
>> http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
>>
>> The idea being, if you move a guest from older QEMU to newer QEMU, but
>> keep the machine type (or more precisely, the full machine config, like
>> the domain XML) intact, the ACPI device node should not appear out of
>> the blue.
> This ACPI device is an always used resource declaration regardless
> of machine type so it makes sense to tell guest about used resource.
>
> The only reason for machine compat code would be if guest suddenly
> started to ask for a driver but as Gabriel showed with _STA(0xB)
> it doesn't, so I'm trying to keep ACPI code machine compat agnostic
> as much as possible.
Fine by me, but I think Eduardo should agree (or disagree) as well,
because it was his point originally.
>
>
> PS:
> is it me alone or email to qemu-devel arrives with huge delay (upto 2 days)?
Several subscribers have reported the same, and I'm seeing delays myself.
Thanks
Laszlo
>
>>
>> I'll let Gabriel answer your other question below. :)
>>
>> Thanks
>> Laszlo
>>
>>
>>>
>>>
+scope = aml_scope("\\_SB");
+dev = aml_device("FWCF");
+
+aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
+/* device present, functioning, decoding, not shown in UI */
+aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
+
+crs = aml_resource_template();
+/* when using port i/o, the 8-bit data register *always* overlaps
+ * with half of the 16-bit control register. Hence, the total size
+ * of the i/o region used is FW_CFG_CTL_SIZE */
+aml_append(crs,
+aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
+ 0x01, FW_CFG_CTL_SIZE)
+);
>>> could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
>>> in any way?
>>>
>>>
+aml_append(dev, aml_name_decl("_CRS", crs));
+
+aml_append(scope, dev);
+aml_append(ssdt, scope);
+}
+
if (misc->applesmc_io_base) {
scope = aml_scope("\\_SB.PCI0.ISA");
dev = aml_device("SMC");
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 3ffb05f..7f5e5d9 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass
*m)
m->alias = NULL;
m->is_default = 0;
pcmc->broken_reserved_end = true;
+pcmc->acpi_no_fw_cfg_node = true;
SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
}
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 1b7d3b6..7180ca3 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
pc_q35_2_5_machine_options(m);
m->alias = NULL;
pcmc->broken_reserved_end = true;
+pcmc->acpi_no_fw_cfg_node = true;
>>>
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On 10/01/15 09:02, Igor Mammedov wrote:
> On Sun, 27 Sep 2015 17:29:00 -0400
> "Gabriel L. Somlo" wrote:
>
>> Add a fw_cfg device node to the ACPI SSDT, on machine types
>> pc-*-2.5 and up. While the guest-side BIOS can't utilize
>> this information (since it has to access the hard-coded
>> fw_cfg device to extract ACPI tables to begin with), having
>> fw_cfg listed in ACPI will help the guest kernel keep a more
>> accurate inventory of in-use IO port regions.
>>
>> Signed-off-by: Gabriel Somlo
>> ---
>> hw/i386/acpi-build.c | 23 +++
>> hw/i386/pc_piix.c| 1 +
>> hw/i386/pc_q35.c | 1 +
>> include/hw/i386/pc.h | 1 +
>> 4 files changed, 26 insertions(+)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 95e0c65..ece2710 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>> PcPciInfo *pci, PcGuestInfo *guest_info)
>> {
>> MachineState *machine = MACHINE(qdev_get_machine());
>> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>> uint32_t nr_mem = machine->ram_slots;
>> unsigned acpi_cpus = guest_info->apic_id_limit;
>> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
>> aml_append(scope, aml_name_decl("_S5", pkg));
>> aml_append(ssdt, scope);
>>
>> +if (!pcmc->acpi_no_fw_cfg_node) {
> we don't really care about SSDT size changes since during
> migration ACPI blobs will be migrated from source, so
> machine compat bloat is excessive here. It would be better
> to just remove it.
This was Eduardo's suggestion, if I recall correctly:
http://thread.gmane.org/gmane.comp.emulators.qemu/361930/focus=361983
The idea being, if you move a guest from older QEMU to newer QEMU, but
keep the machine type (or more precisely, the full machine config, like
the domain XML) intact, the ACPI device node should not appear out of
the blue.
I'll let Gabriel answer your other question below. :)
Thanks
Laszlo
>
>
>> +scope = aml_scope("\\_SB");
>> +dev = aml_device("FWCF");
>> +
>> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
>> +/* device present, functioning, decoding, not shown in UI */
>> +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
>> +
>> +crs = aml_resource_template();
>> +/* when using port i/o, the 8-bit data register *always* overlaps
>> + * with half of the 16-bit control register. Hence, the total size
>> + * of the i/o region used is FW_CFG_CTL_SIZE */
>> +aml_append(crs,
>> +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
>> + 0x01, FW_CFG_CTL_SIZE)
>> +);
> could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
> in any way?
>
>
>> +aml_append(dev, aml_name_decl("_CRS", crs));
>> +
>> +aml_append(scope, dev);
>> +aml_append(ssdt, scope);
>> +}
>> +
>> if (misc->applesmc_io_base) {
>> scope = aml_scope("\\_SB.PCI0.ISA");
>> dev = aml_device("SMC");
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 3ffb05f..7f5e5d9 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass
>> *m)
>> m->alias = NULL;
>> m->is_default = 0;
>> pcmc->broken_reserved_end = true;
>> +pcmc->acpi_no_fw_cfg_node = true;
>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>> }
>>
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 1b7d3b6..7180ca3 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>> pc_q35_2_5_machine_options(m);
>> m->alias = NULL;
>> pcmc->broken_reserved_end = true;
>> +pcmc->acpi_no_fw_cfg_node = true;
>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>> }
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 86007e3..6d0f1bd 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -62,6 +62,7 @@ struct PCMachineClass {
>> bool broken_reserved_end;
>> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
>> DeviceState *dev);
>> +bool acpi_no_fw_cfg_node;
>> };
>>
>> #define TYPE_PC_MACHINE "generic-pc-machine"
>
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Sun, 27 Sep 2015 17:29:00 -0400
"Gabriel L. Somlo" wrote:
> Add a fw_cfg device node to the ACPI SSDT, on machine types
> pc-*-2.5 and up. While the guest-side BIOS can't utilize
> this information (since it has to access the hard-coded
> fw_cfg device to extract ACPI tables to begin with), having
> fw_cfg listed in ACPI will help the guest kernel keep a more
> accurate inventory of in-use IO port regions.
>
> Signed-off-by: Gabriel Somlo
> ---
> hw/i386/acpi-build.c | 23 +++
> hw/i386/pc_piix.c| 1 +
> hw/i386/pc_q35.c | 1 +
> include/hw/i386/pc.h | 1 +
> 4 files changed, 26 insertions(+)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..ece2710 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> PcPciInfo *pci, PcGuestInfo *guest_info)
> {
> MachineState *machine = MACHINE(qdev_get_machine());
> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> uint32_t nr_mem = machine->ram_slots;
> unsigned acpi_cpus = guest_info->apic_id_limit;
> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> aml_append(scope, aml_name_decl("_S5", pkg));
> aml_append(ssdt, scope);
>
> +if (!pcmc->acpi_no_fw_cfg_node) {
we don't really care about SSDT size changes since during
migration ACPI blobs will be migrated from source, so
machine compat bloat is excessive here. It would be better
to just remove it.
> +scope = aml_scope("\\_SB");
> +dev = aml_device("FWCF");
> +
> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +/* device present, functioning, decoding, not shown in UI */
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +crs = aml_resource_template();
> +/* when using port i/o, the 8-bit data register *always* overlaps
> + * with half of the 16-bit control register. Hence, the total size
> + * of the i/o region used is FW_CFG_CTL_SIZE */
> +aml_append(crs,
> +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> + 0x01, FW_CFG_CTL_SIZE)
> +);
could you check/dump this _CRS and PCI0._CRS to see if they are intersecting
in any way?
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +aml_append(scope, dev);
> +aml_append(ssdt, scope);
> +}
> +
> if (misc->applesmc_io_base) {
> scope = aml_scope("\\_SB.PCI0.ISA");
> dev = aml_device("SMC");
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 3ffb05f..7f5e5d9 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
> m->alias = NULL;
> m->is_default = 0;
> pcmc->broken_reserved_end = true;
> +pcmc->acpi_no_fw_cfg_node = true;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> }
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 1b7d3b6..7180ca3 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> pc_q35_2_5_machine_options(m);
> m->alias = NULL;
> pcmc->broken_reserved_end = true;
> +pcmc->acpi_no_fw_cfg_node = true;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> }
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 86007e3..6d0f1bd 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -62,6 +62,7 @@ struct PCMachineClass {
> bool broken_reserved_end;
> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> DeviceState *dev);
> +bool acpi_no_fw_cfg_node;
> };
>
> #define TYPE_PC_MACHINE "generic-pc-machine"
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On 30/09/2015 16:16, Gabriel L. Somlo wrote: >>> > > Yes, we're OK. Throughout it all I *meant* to write 0x0B (bee), but my >>> > > brain sometimes mistakenly makes me write 0x08 (eight) instead. Sorry >>> > > for >>> > > the confusion... :) >> > >> > IIRC from the pvpanic trainwreck, Windows XP and 2003 always complain >> > even for 0x0B about a missing driver. > Don't have 2003, but dug up and started my old xp_sp3 image, and > Device Manager only complains when _STA defaults to 0x0f (i.e. when > I omit generating it in aml completely). > > With _STA set to 0x0b (like in the latest version of the patch), the > fw_cfg device does not show up with a missing device complaint on xp. Great, thanks for testing. Paolo
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Wed, Sep 30, 2015 at 03:01:13PM +0200, Paolo Bonzini wrote: > > > On 30/09/2015 02:18, Gabriel L. Somlo wrote: > > Yes, we're OK. Throughout it all I *meant* to write 0x0B (bee), but my > > brain sometimes mistakenly makes me write 0x08 (eight) instead. Sorry for > > the confusion... :) > > IIRC from the pvpanic trainwreck, Windows XP and 2003 always complain > even for 0x0B about a missing driver. Don't have 2003, but dug up and started my old xp_sp3 image, and Device Manager only complains when _STA defaults to 0x0f (i.e. when I omit generating it in aml completely). With _STA set to 0x0b (like in the latest version of the patch), the fw_cfg device does not show up with a missing device complaint on xp. Thanks, --Gabriel
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On 30/09/2015 02:18, Gabriel L. Somlo wrote: > Yes, we're OK. Throughout it all I *meant* to write 0x0B (bee), but my > brain sometimes mistakenly makes me write 0x08 (eight) instead. Sorry for > the confusion... :) IIRC from the pvpanic trainwreck, Windows XP and 2003 always complain even for 0x0B about a missing driver. Paolo
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Tue, Sep 29, 2015 at 07:28:39PM +0200, Laszlo Ersek wrote:
> On 09/29/15 19:19, Gabriel L. Somlo wrote:
> > On Tue, Sep 29, 2015 at 06:55:01PM +0200, Laszlo Ersek wrote:
> >> On 09/29/15 18:46, Gabriel L. Somlo wrote:
> >>> On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
> On 09/27/15 23:29, Gabriel L. Somlo wrote:
> > Add a fw_cfg device node to the ACPI SSDT, on machine types
> > pc-*-2.5 and up. While the guest-side BIOS can't utilize
> > this information (since it has to access the hard-coded
> > fw_cfg device to extract ACPI tables to begin with), having
> > fw_cfg listed in ACPI will help the guest kernel keep a more
> > accurate inventory of in-use IO port regions.
> >
> > Signed-off-by: Gabriel Somlo
> > ---
> > hw/i386/acpi-build.c | 23 +++
> > hw/i386/pc_piix.c| 1 +
> > hw/i386/pc_q35.c | 1 +
> > include/hw/i386/pc.h | 1 +
> > 4 files changed, 26 insertions(+)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 95e0c65..ece2710 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > PcPciInfo *pci, PcGuestInfo *guest_info)
> > {
> > MachineState *machine = MACHINE(qdev_get_machine());
> > +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > uint32_t nr_mem = machine->ram_slots;
> > unsigned acpi_cpus = guest_info->apic_id_limit;
> > Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
> > *ifctx;
> > @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> > aml_append(scope, aml_name_decl("_S5", pkg));
> > aml_append(ssdt, scope);
> >
> > +if (!pcmc->acpi_no_fw_cfg_node) {
> > +scope = aml_scope("\\_SB");
> > +dev = aml_device("FWCF");
> > +
> > +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> > +/* device present, functioning, decoding, not shown in UI */
> > +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> > +
> > +crs = aml_resource_template();
> > +/* when using port i/o, the 8-bit data register *always*
> > overlaps
> > + * with half of the 16-bit control register. Hence, the total
> > size
> > + * of the i/o region used is FW_CFG_CTL_SIZE */
> > +aml_append(crs,
> > +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> > + 0x01, FW_CFG_CTL_SIZE)
> > +);
>
> I think "aml_io" should be indented so that it lines up with "crs" above
> it.
> >>>
> >>> There are a few other nodes being added in if() {...} bloks
> >>> immediately following the fw_cfg one. They *all* indent it this way, I
> >>> just made mine look similar. That said, I have no problem indenting
> >>> mine differently, if you still want me to... :)
> >>
> >> Nah, if you are consistent with existing code there, I'm fine.
> >>
> >>>
>
> Other than that:
>
> Reviewed-by: Laszlo Ersek
>
> What Windows guests did you test this with? ("Testing" meant as "looked
> at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
> that.
> >>>
> >>> I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
> >>> complains about not being able to find a driver for it :)
> >>
> >> So you had to clear bit 0 (value 1, "device is present") and bit 1
> >> (value 2, "device is enabled and decoding its resources") in _STA,
> >> relative to 0xB visible above? I'm not sure if that's a good thing.
> >> First, setting bit 3 (value 8, "device is functioning properly"0 without
> >> the device being present is... strange. Second, won't that prevent you
> >> from using the resources even in the Linux driver?
> >
> > no, 0x0B is 1011, the only bit I'm clearing is "shown in the u/i".
> > Leaving out _STA entirely would have had it default to 0x0F, and the
> > "show in u/i" bit caused Windows to show it in the device manager with
> > a yellow excalmation sign... So I had to go back and add an explicit
> > _STA with the u/i bit turned off.
>
> Ah okay. So when you wrote "re-adding _STA set to 0x08", you actually
> meant *this* patch. Right? (I don't really understand the reference to
> 0x08.)
>
> So I take you tested *this* patch with Windows 7, and it was satisfied.
> Good.
Yes, we're OK. Throughout it all I *meant* to write 0x0B (bee), but my
brain sometimes mistakenly makes me write 0x08 (eight) instead. Sorry for
the confusion... :)
Thanks,
--Gabriel
>
> >
> >> (My working assumption is that you're doing this for QEMU because GregKH
> >> (IIRC?) told you that the kernel driver should be keying off of ACPI. Is
> >> that right?)
> >
> > First, to answer mst's question elswhere in
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Tue, Sep 29, 2015 at 06:55:01PM +0200, Laszlo Ersek wrote:
> On 09/29/15 18:46, Gabriel L. Somlo wrote:
> > On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
> >> On 09/27/15 23:29, Gabriel L. Somlo wrote:
> >>> Add a fw_cfg device node to the ACPI SSDT, on machine types
> >>> pc-*-2.5 and up. While the guest-side BIOS can't utilize
> >>> this information (since it has to access the hard-coded
> >>> fw_cfg device to extract ACPI tables to begin with), having
> >>> fw_cfg listed in ACPI will help the guest kernel keep a more
> >>> accurate inventory of in-use IO port regions.
> >>>
> >>> Signed-off-by: Gabriel Somlo
> >>> ---
> >>> hw/i386/acpi-build.c | 23 +++
> >>> hw/i386/pc_piix.c| 1 +
> >>> hw/i386/pc_q35.c | 1 +
> >>> include/hw/i386/pc.h | 1 +
> >>> 4 files changed, 26 insertions(+)
> >>>
> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >>> index 95e0c65..ece2710 100644
> >>> --- a/hw/i386/acpi-build.c
> >>> +++ b/hw/i386/acpi-build.c
> >>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>> PcPciInfo *pci, PcGuestInfo *guest_info)
> >>> {
> >>> MachineState *machine = MACHINE(qdev_get_machine());
> >>> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> >>> uint32_t nr_mem = machine->ram_slots;
> >>> unsigned acpi_cpus = guest_info->apic_id_limit;
> >>> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
> >>> *ifctx;
> >>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> >>> aml_append(scope, aml_name_decl("_S5", pkg));
> >>> aml_append(ssdt, scope);
> >>>
> >>> +if (!pcmc->acpi_no_fw_cfg_node) {
> >>> +scope = aml_scope("\\_SB");
> >>> +dev = aml_device("FWCF");
> >>> +
> >>> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> >>> +/* device present, functioning, decoding, not shown in UI */
> >>> +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> >>> +
> >>> +crs = aml_resource_template();
> >>> +/* when using port i/o, the 8-bit data register *always* overlaps
> >>> + * with half of the 16-bit control register. Hence, the total
> >>> size
> >>> + * of the i/o region used is FW_CFG_CTL_SIZE */
> >>> +aml_append(crs,
> >>> +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> >>> + 0x01, FW_CFG_CTL_SIZE)
> >>> +);
> >>
> >> I think "aml_io" should be indented so that it lines up with "crs" above
> >> it.
> >
> > There are a few other nodes being added in if() {...} bloks
> > immediately following the fw_cfg one. They *all* indent it this way, I
> > just made mine look similar. That said, I have no problem indenting
> > mine differently, if you still want me to... :)
>
> Nah, if you are consistent with existing code there, I'm fine.
>
> >
> >>
> >> Other than that:
> >>
> >> Reviewed-by: Laszlo Ersek
> >>
> >> What Windows guests did you test this with? ("Testing" meant as "looked
> >> at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
> >> that.
> >
> > I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
> > complains about not being able to find a driver for it :)
>
> So you had to clear bit 0 (value 1, "device is present") and bit 1
> (value 2, "device is enabled and decoding its resources") in _STA,
> relative to 0xB visible above? I'm not sure if that's a good thing.
> First, setting bit 3 (value 8, "device is functioning properly"0 without
> the device being present is... strange. Second, won't that prevent you
> from using the resources even in the Linux driver?
no, 0x0B is 1011, the only bit I'm clearing is "shown in the u/i".
Leaving out _STA entirely would have had it default to 0x0F, and the
"show in u/i" bit caused Windows to show it in the device manager with
a yellow excalmation sign... So I had to go back and add an explicit
_STA with the u/i bit turned off.
> (My working assumption is that you're doing this for QEMU because GregKH
> (IIRC?) told you that the kernel driver should be keying off of ACPI. Is
> that right?)
First, to answer mst's question elswhere in this thread, I'm
working on a kernel sysfs driver that would list fw_cfg blobs in
sysfs (just like /sys/firmware/dmi/entries/...) so userspace could
simply "cat" or "cp" the raw blobs.
GregKH told me to try udev for the friendly path blobname expansion
(your "I like using find on /sys/firmware..." recommendation). He never
said anything about ACPI (not sure he would have eventually or not).
It all started with ardb saying "NAK on arm if you touch the mmio
registers before checking with DT that you even have a fw_cfg device".
I sort-of figured I'd better not touch IOport registers either before
I know I have a fw_cfg device, hence this whole exercise of adding it
to ACPI. Although I still have to figure out how one would do
something like
if (
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On 09/29/15 18:46, Gabriel L. Somlo wrote:
> On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
>> On 09/27/15 23:29, Gabriel L. Somlo wrote:
>>> Add a fw_cfg device node to the ACPI SSDT, on machine types
>>> pc-*-2.5 and up. While the guest-side BIOS can't utilize
>>> this information (since it has to access the hard-coded
>>> fw_cfg device to extract ACPI tables to begin with), having
>>> fw_cfg listed in ACPI will help the guest kernel keep a more
>>> accurate inventory of in-use IO port regions.
>>>
>>> Signed-off-by: Gabriel Somlo
>>> ---
>>> hw/i386/acpi-build.c | 23 +++
>>> hw/i386/pc_piix.c| 1 +
>>> hw/i386/pc_q35.c | 1 +
>>> include/hw/i386/pc.h | 1 +
>>> 4 files changed, 26 insertions(+)
>>>
>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>>> index 95e0c65..ece2710 100644
>>> --- a/hw/i386/acpi-build.c
>>> +++ b/hw/i386/acpi-build.c
>>> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>> PcPciInfo *pci, PcGuestInfo *guest_info)
>>> {
>>> MachineState *machine = MACHINE(qdev_get_machine());
>>> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
>>> uint32_t nr_mem = machine->ram_slots;
>>> unsigned acpi_cpus = guest_info->apic_id_limit;
>>> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
>>> *ifctx;
>>> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
>>> aml_append(scope, aml_name_decl("_S5", pkg));
>>> aml_append(ssdt, scope);
>>>
>>> +if (!pcmc->acpi_no_fw_cfg_node) {
>>> +scope = aml_scope("\\_SB");
>>> +dev = aml_device("FWCF");
>>> +
>>> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
>>> +/* device present, functioning, decoding, not shown in UI */
>>> +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
>>> +
>>> +crs = aml_resource_template();
>>> +/* when using port i/o, the 8-bit data register *always* overlaps
>>> + * with half of the 16-bit control register. Hence, the total size
>>> + * of the i/o region used is FW_CFG_CTL_SIZE */
>>> +aml_append(crs,
>>> +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
>>> + 0x01, FW_CFG_CTL_SIZE)
>>> +);
>>
>> I think "aml_io" should be indented so that it lines up with "crs" above it.
>
> There are a few other nodes being added in if() {...} bloks
> immediately following the fw_cfg one. They *all* indent it this way, I
> just made mine look similar. That said, I have no problem indenting
> mine differently, if you still want me to... :)
Nah, if you are consistent with existing code there, I'm fine.
>
>>
>> Other than that:
>>
>> Reviewed-by: Laszlo Ersek
>>
>> What Windows guests did you test this with? ("Testing" meant as "looked
>> at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
>> that.
>
> I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
> complains about not being able to find a driver for it :)
So you had to clear bit 0 (value 1, "device is present") and bit 1
(value 2, "device is enabled and decoding its resources") in _STA,
relative to 0xB visible above? I'm not sure if that's a good thing.
First, setting bit 3 (value 8, "device is functioning properly"0 without
the device being present is... strange. Second, won't that prevent you
from using the resources even in the Linux driver?
(My working assumption is that you're doing this for QEMU because GregKH
(IIRC?) told you that the kernel driver should be keying off of ACPI. Is
that right?)
Thanks!
Laszlo
>
> Thanks,
> --Gabriel
>
>>
>>> +aml_append(dev, aml_name_decl("_CRS", crs));
>>> +
>>> +aml_append(scope, dev);
>>> +aml_append(ssdt, scope);
>>> +}
>>> +
>>> if (misc->applesmc_io_base) {
>>> scope = aml_scope("\\_SB.PCI0.ISA");
>>> dev = aml_device("SMC");
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index 3ffb05f..7f5e5d9 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass
>>> *m)
>>> m->alias = NULL;
>>> m->is_default = 0;
>>> pcmc->broken_reserved_end = true;
>>> +pcmc->acpi_no_fw_cfg_node = true;
>>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>> }
>>>
>>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>>> index 1b7d3b6..7180ca3 100644
>>> --- a/hw/i386/pc_q35.c
>>> +++ b/hw/i386/pc_q35.c
>>> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
>>> pc_q35_2_5_machine_options(m);
>>> m->alias = NULL;
>>> pcmc->broken_reserved_end = true;
>>> +pcmc->acpi_no_fw_cfg_node = true;
>>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
>>> }
>>>
>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>>> index 86007e3..6d0f1bd 100644
>>> --- a/include/hw/i386/pc.h
>>> +++ b/include/hw/i386/pc.h
>>> @@
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On 09/29/15 19:19, Gabriel L. Somlo wrote:
> On Tue, Sep 29, 2015 at 06:55:01PM +0200, Laszlo Ersek wrote:
>> On 09/29/15 18:46, Gabriel L. Somlo wrote:
>>> On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
On 09/27/15 23:29, Gabriel L. Somlo wrote:
> Add a fw_cfg device node to the ACPI SSDT, on machine types
> pc-*-2.5 and up. While the guest-side BIOS can't utilize
> this information (since it has to access the hard-coded
> fw_cfg device to extract ACPI tables to begin with), having
> fw_cfg listed in ACPI will help the guest kernel keep a more
> accurate inventory of in-use IO port regions.
>
> Signed-off-by: Gabriel Somlo
> ---
> hw/i386/acpi-build.c | 23 +++
> hw/i386/pc_piix.c| 1 +
> hw/i386/pc_q35.c | 1 +
> include/hw/i386/pc.h | 1 +
> 4 files changed, 26 insertions(+)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..ece2710 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> PcPciInfo *pci, PcGuestInfo *guest_info)
> {
> MachineState *machine = MACHINE(qdev_get_machine());
> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> uint32_t nr_mem = machine->ram_slots;
> unsigned acpi_cpus = guest_info->apic_id_limit;
> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
> *ifctx;
> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> aml_append(scope, aml_name_decl("_S5", pkg));
> aml_append(ssdt, scope);
>
> +if (!pcmc->acpi_no_fw_cfg_node) {
> +scope = aml_scope("\\_SB");
> +dev = aml_device("FWCF");
> +
> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +/* device present, functioning, decoding, not shown in UI */
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +crs = aml_resource_template();
> +/* when using port i/o, the 8-bit data register *always* overlaps
> + * with half of the 16-bit control register. Hence, the total
> size
> + * of the i/o region used is FW_CFG_CTL_SIZE */
> +aml_append(crs,
> +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> + 0x01, FW_CFG_CTL_SIZE)
> +);
I think "aml_io" should be indented so that it lines up with "crs" above
it.
>>>
>>> There are a few other nodes being added in if() {...} bloks
>>> immediately following the fw_cfg one. They *all* indent it this way, I
>>> just made mine look similar. That said, I have no problem indenting
>>> mine differently, if you still want me to... :)
>>
>> Nah, if you are consistent with existing code there, I'm fine.
>>
>>>
Other than that:
Reviewed-by: Laszlo Ersek
What Windows guests did you test this with? ("Testing" meant as "looked
at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
that.
>>>
>>> I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
>>> complains about not being able to find a driver for it :)
>>
>> So you had to clear bit 0 (value 1, "device is present") and bit 1
>> (value 2, "device is enabled and decoding its resources") in _STA,
>> relative to 0xB visible above? I'm not sure if that's a good thing.
>> First, setting bit 3 (value 8, "device is functioning properly"0 without
>> the device being present is... strange. Second, won't that prevent you
>> from using the resources even in the Linux driver?
>
> no, 0x0B is 1011, the only bit I'm clearing is "shown in the u/i".
> Leaving out _STA entirely would have had it default to 0x0F, and the
> "show in u/i" bit caused Windows to show it in the device manager with
> a yellow excalmation sign... So I had to go back and add an explicit
> _STA with the u/i bit turned off.
Ah okay. So when you wrote "re-adding _STA set to 0x08", you actually
meant *this* patch. Right? (I don't really understand the reference to
0x08.)
So I take you tested *this* patch with Windows 7, and it was satisfied.
Good.
>
>> (My working assumption is that you're doing this for QEMU because GregKH
>> (IIRC?) told you that the kernel driver should be keying off of ACPI. Is
>> that right?)
>
> First, to answer mst's question elswhere in this thread, I'm
> working on a kernel sysfs driver that would list fw_cfg blobs in
> sysfs (just like /sys/firmware/dmi/entries/...) so userspace could
> simply "cat" or "cp" the raw blobs.
>
> GregKH told me to try udev for the friendly path blobname expansion
> (your "I like using find on /sys/firmware..." recommendation). He never
> said anything about ACPI (not sure he would have eventually or not).
>
> It all started with ardb saying "NAK on arm if you
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On Tue, Sep 29, 2015 at 12:33:40PM +0200, Laszlo Ersek wrote:
> On 09/27/15 23:29, Gabriel L. Somlo wrote:
> > Add a fw_cfg device node to the ACPI SSDT, on machine types
> > pc-*-2.5 and up. While the guest-side BIOS can't utilize
> > this information (since it has to access the hard-coded
> > fw_cfg device to extract ACPI tables to begin with), having
> > fw_cfg listed in ACPI will help the guest kernel keep a more
> > accurate inventory of in-use IO port regions.
> >
> > Signed-off-by: Gabriel Somlo
> > ---
> > hw/i386/acpi-build.c | 23 +++
> > hw/i386/pc_piix.c| 1 +
> > hw/i386/pc_q35.c | 1 +
> > include/hw/i386/pc.h | 1 +
> > 4 files changed, 26 insertions(+)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 95e0c65..ece2710 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > PcPciInfo *pci, PcGuestInfo *guest_info)
> > {
> > MachineState *machine = MACHINE(qdev_get_machine());
> > +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > uint32_t nr_mem = machine->ram_slots;
> > unsigned acpi_cpus = guest_info->apic_id_limit;
> > Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field,
> > *ifctx;
> > @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> > aml_append(scope, aml_name_decl("_S5", pkg));
> > aml_append(ssdt, scope);
> >
> > +if (!pcmc->acpi_no_fw_cfg_node) {
> > +scope = aml_scope("\\_SB");
> > +dev = aml_device("FWCF");
> > +
> > +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> > +/* device present, functioning, decoding, not shown in UI */
> > +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> > +
> > +crs = aml_resource_template();
> > +/* when using port i/o, the 8-bit data register *always* overlaps
> > + * with half of the 16-bit control register. Hence, the total size
> > + * of the i/o region used is FW_CFG_CTL_SIZE */
> > +aml_append(crs,
> > +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> > + 0x01, FW_CFG_CTL_SIZE)
> > +);
>
> I think "aml_io" should be indented so that it lines up with "crs" above it.
There are a few other nodes being added in if() {...} bloks
immediately following the fw_cfg one. They *all* indent it this way, I
just made mine look similar. That said, I have no problem indenting
mine differently, if you still want me to... :)
>
> Other than that:
>
> Reviewed-by: Laszlo Ersek
>
> What Windows guests did you test this with? ("Testing" meant as "looked
> at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
> that.
I tested on winddows 7. After re-adding _STA set to 0x08, it no longer
complains about not being able to find a driver for it :)
Thanks,
--Gabriel
>
> > +aml_append(dev, aml_name_decl("_CRS", crs));
> > +
> > +aml_append(scope, dev);
> > +aml_append(ssdt, scope);
> > +}
> > +
> > if (misc->applesmc_io_base) {
> > scope = aml_scope("\\_SB.PCI0.ISA");
> > dev = aml_device("SMC");
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 3ffb05f..7f5e5d9 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass
> > *m)
> > m->alias = NULL;
> > m->is_default = 0;
> > pcmc->broken_reserved_end = true;
> > +pcmc->acpi_no_fw_cfg_node = true;
> > SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> > }
> >
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 1b7d3b6..7180ca3 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> > pc_q35_2_5_machine_options(m);
> > m->alias = NULL;
> > pcmc->broken_reserved_end = true;
> > +pcmc->acpi_no_fw_cfg_node = true;
> > SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> > }
> >
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 86007e3..6d0f1bd 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -62,6 +62,7 @@ struct PCMachineClass {
> > bool broken_reserved_end;
> > HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > DeviceState *dev);
> > +bool acpi_no_fw_cfg_node;
> > };
> >
> > #define TYPE_PC_MACHINE "generic-pc-machine"
> >
>
Re: [Qemu-devel] [PATCH v4 3/5] acpi: pc: add fw_cfg device node to ssdt
On 09/27/15 23:29, Gabriel L. Somlo wrote:
> Add a fw_cfg device node to the ACPI SSDT, on machine types
> pc-*-2.5 and up. While the guest-side BIOS can't utilize
> this information (since it has to access the hard-coded
> fw_cfg device to extract ACPI tables to begin with), having
> fw_cfg listed in ACPI will help the guest kernel keep a more
> accurate inventory of in-use IO port regions.
>
> Signed-off-by: Gabriel Somlo
> ---
> hw/i386/acpi-build.c | 23 +++
> hw/i386/pc_piix.c| 1 +
> hw/i386/pc_q35.c | 1 +
> include/hw/i386/pc.h | 1 +
> 4 files changed, 26 insertions(+)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 95e0c65..ece2710 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -906,6 +906,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> PcPciInfo *pci, PcGuestInfo *guest_info)
> {
> MachineState *machine = MACHINE(qdev_get_machine());
> +PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> uint32_t nr_mem = machine->ram_slots;
> unsigned acpi_cpus = guest_info->apic_id_limit;
> Aml *ssdt, *sb_scope, *scope, *pkg, *dev, *method, *crs, *field, *ifctx;
> @@ -1071,6 +1072,28 @@ build_ssdt(GArray *table_data, GArray *linker,
> aml_append(scope, aml_name_decl("_S5", pkg));
> aml_append(ssdt, scope);
>
> +if (!pcmc->acpi_no_fw_cfg_node) {
> +scope = aml_scope("\\_SB");
> +dev = aml_device("FWCF");
> +
> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +/* device present, functioning, decoding, not shown in UI */
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +crs = aml_resource_template();
> +/* when using port i/o, the 8-bit data register *always* overlaps
> + * with half of the 16-bit control register. Hence, the total size
> + * of the i/o region used is FW_CFG_CTL_SIZE */
> +aml_append(crs,
> +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE,
> + 0x01, FW_CFG_CTL_SIZE)
> +);
I think "aml_io" should be indented so that it lines up with "crs" above it.
Other than that:
Reviewed-by: Laszlo Ersek
What Windows guests did you test this with? ("Testing" meant as "looked
at Device Manager".) I can help with Windows 7, 8, and 10, if you'd like
that.
Thanks
Laszlo
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +aml_append(scope, dev);
> +aml_append(ssdt, scope);
> +}
> +
> if (misc->applesmc_io_base) {
> scope = aml_scope("\\_SB.PCI0.ISA");
> dev = aml_device("SMC");
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 3ffb05f..7f5e5d9 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -482,6 +482,7 @@ static void pc_i440fx_2_4_machine_options(MachineClass *m)
> m->alias = NULL;
> m->is_default = 0;
> pcmc->broken_reserved_end = true;
> +pcmc->acpi_no_fw_cfg_node = true;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> }
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 1b7d3b6..7180ca3 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -385,6 +385,7 @@ static void pc_q35_2_4_machine_options(MachineClass *m)
> pc_q35_2_5_machine_options(m);
> m->alias = NULL;
> pcmc->broken_reserved_end = true;
> +pcmc->acpi_no_fw_cfg_node = true;
> SET_MACHINE_COMPAT(m, PC_COMPAT_2_4);
> }
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 86007e3..6d0f1bd 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -62,6 +62,7 @@ struct PCMachineClass {
> bool broken_reserved_end;
> HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> DeviceState *dev);
> +bool acpi_no_fw_cfg_node;
> };
>
> #define TYPE_PC_MACHINE "generic-pc-machine"
>
