On 05/28/15 23:08, Gabriel L. Somlo wrote: > On Thu, May 28, 2015 at 10:04:07PM +0200, Laszlo Ersek wrote: >> Version 2 of >> <http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg05640.html>. >> >> Changes are broken out per-patch; the cumulative changes are: >> - more granular structure (several patches in place of 1), >> - rename "force_fdctrl" parameter to "create_fdctrl", >> - drop the separate compat knob "force_fdctrl", use the "no_floppy" >> machine class setting in its stead (in inverse meaning). >> >> I didn't touch ACPI bits (raised by Gabriel) because I got the >> impression that they are >> - alright on PIIX4 (which sees no change in this series), and >> - already handled correctly / dynamically on Q35 (an independent issue >> was discovered but Gerd took that on, thanks). > > With your patches, I started an F21 guest like so: > > bin/qemu-system-x86_64 -machine q35,accel=kvm -m 2048 -monitor stdio -device > ide-drive,bus=ide.2,drive=CD -drive > id=CD,if=none,snapshot=on,file=/home/somlo/Downloads/Iso/Fedora-Live-Workstation-x86_64-21-5.iso > > I then installed acpica-tools, and dumped out the DSDT > (acpidump -b; iasl -d dsdt.dat) > > Looking at dsdt.dsl, I found this: > > ... > Scope (_SB.PCI0) > { > Device (ISA) > { > ... > OperationRegion (LPCE, PCI_Config, 0x82, 0x02) > Field (LPCE, AnyAcc, NoLock, Preserve) > { > ... > FDEN, 1 > } > } > } > > Scope (_SB.PCI0.ISA) > { > ... > Device (FDC0) > { > Name (_HID, EisaId ("PNP0700")) // _HID: Hardware ID > Method (_STA, 0, NotSerialized) // _STA: Status > { > Local0 = FDEN /* \_SB_.PCI0.ISA_.FDEN */ > If ((Local0 == Zero)) > { > Return (Zero) > } > Else > { > Return (0x0F) > } > } > > Name (_CRS, ResourceTemplate () // _CRS: Current Resource > Settings > { > IO (Decode16, > 0x03F2, // Range Minimum > 0x03F2, // Range Maximum > 0x00, // Alignment > 0x04, // Length > ) > IO (Decode16, > 0x03F7, // Range Minimum > 0x03F7, // Range Maximum > 0x00, // Alignment > 0x01, // Length > ) > IRQNoFlags () > {6} > DMA (Compatibility, NotBusMaster, Transfer8, ) > {2} > }) > } > ... > > I have no idea how big of a deal this is (i.e. how "wrong" it is for > this stuff to still be showing up when no FDC is provisioned on the > guest).
The _STA method will return 0 if the FDEN field is zero. In the value returned by _STA (which is a bitmask), bit 0 is "Set if the device is present.". Since FDEN==0 implies that this bit in the retval of _STA will be zero, we can conclude that FDEN==0 implies that "the FDC0 device is absent". So presenting the payload is not a problem; when OSPM evaluates _STA, it will see that the device doesn't exist. The question is if FDEN is zero under these circumstances. The LPCE operation region overlays the PCI config space of the "PCI D31:f0 LPC ISA bridge" device -- see the _ADR object --, starting at offset 0x82 in the config space, and continuing for 2 bytes. Within this region, FDEN denotes bit#3 of the byte at the lowest address. (The bit offset that FDEN corresponds to, and the _ADR object, are not visible in the context that you quoted, but they can be seen in "hw/i386/q35-acpi-dsdt.dsl".) In "hw/isa/lpc_ich9.c", function ich9_lpc_machine_ready(), we have: if (memory_region_present(io_as, 0x3f0)) { /* floppy */ pci_conf[0x82] |= 0x08; } That is, the FDEN bit will evaluate to 1 in the _STA method only if the above memory_region_present() call returned "true" at machine startup. That IO port is set up in the following call chain: pc_basic_device_init() [hw/i386/pc.c] fdctrl_init_isa() [hw/block/fdc.c] qdev_init_nofail() [hw/core/qdev.c] ... isabus_fdc_realize() [hw/block/fdc.c] // iobase = 0x3f0 comes from // "isa_fdc_properties" isa_register_portio_list() [hw/isa/isa-bus.c] portio_list_add() [ioport.c] portio_list_add_1() [ioport.c] memory_region_init_io() [memory.c] memory_region_add_subregion() [memory.c] This patch series prevents the pc_basic_device_init() --> fdctrl_init_isa() call at the top, under the right circumstances. Hence \_SB.PCI0.ISA.FDC0._STA() will report "device absent". (I'm just repeating Gerd's earlier explanation, with more details.) Thanks Laszlo > > In any case, the patch below adds a FDC0 node (to the ssdt rather than > the dsdt -- I used the applesmc as a source of inspiration) to acpi > only if one is actually configured on the system. > > I had to add an "aml_dma()" function first (that's the first two > diff's in the patch), then I'm programmatically and conditionally > adding AML for the FDC0 node (next two diff blocks), and lastly I'm > removing the hardcoded node from the .dsl files. > > I can split these out properly and send real patches, but wanted to > give you all a chance to talk me down, in case I'm still missing the > point somehow :) > > Thanks much, > --Gabriel > > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index 3947201..93e4244 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -173,6 +173,7 @@ Aml *aml_io(AmlIODecode dec, uint16_t min_base, uint16_t > max_base, > Aml *aml_operation_region(const char *name, AmlRegionSpace rs, > uint32_t offset, uint32_t len); > Aml *aml_irq_no_flags(uint8_t irq); > +Aml *aml_dma(uint8_t type, bool is_bus_master, uint8_t size, uint8_t > channel); > Aml *aml_named_field(const char *name, unsigned length); > Aml *aml_reserved_field(unsigned length); > Aml *aml_local(int num); > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index 77ce00b..f3380dd 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -542,6 +542,25 @@ Aml *aml_irq_no_flags(uint8_t irq) > return var; > } > > +Aml *aml_dma(uint8_t type, bool is_bus_master, uint8_t size, uint8_t channel) > +{ > + uint8_t dma_mask; > + Aml *var = aml_alloc(); > + > + assert(type < 4); > + assert(size < 4); > + assert(channel < 8); > + build_append_byte(var->buf, 0x2A); /* DMA descriptor */ > + > + dma_mask = 1U << channel; > + build_append_byte(var->buf, dma_mask); > + > + build_append_byte(var->buf, (type << 5) | > + is_bus_master ? 1U << 2 : 0 | > + size); > + return var; > +} > + > /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefLEqual */ > Aml *aml_equal(Aml *arg1, Aml *arg2) > { > diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h > index d48b2f8..f82d3e6 100644 > --- a/include/hw/block/fdc.h > +++ b/include/hw/block/fdc.h > @@ -14,6 +14,17 @@ typedef enum FDriveType { > } FDriveType; > > #define TYPE_ISA_FDC "isa-fdc" > +#define ISA_FDC_PROP_IO_BASE "iobase" > + > +static inline uint16_t isafdc_port(void) > +{ > + Object *obj = object_resolve_path_type("", TYPE_ISA_FDC, NULL); > + > + if (obj) { > + return object_property_get_int(obj, ISA_FDC_PROP_IO_BASE, NULL); > + } > + return 0; > +} > > ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds); > void fdctrl_init_sysbus(qemu_irq irq, int dma_chann, > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 73259e7..cfa275f 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -111,6 +111,7 @@ typedef struct AcpiMiscInfo { > unsigned dsdt_size; > uint16_t pvpanic_port; > uint16_t applesmc_io_base; > + uint16_t isafdc_io_base; > } AcpiMiscInfo; > > typedef struct AcpiBuildPciBusHotplugState { > @@ -237,6 +238,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info) > info->has_tpm = tpm_find(); > info->pvpanic_port = pvpanic_port(); > info->applesmc_io_base = applesmc_port(); > + info->isafdc_io_base = isafdc_port(); > } > > static void acpi_get_pci_info(PcPciInfo *info) > @@ -730,6 +732,30 @@ build_ssdt(GArray *table_data, GArray *linker, > aml_append(ssdt, scope); > } > > + if (misc->isafdc_io_base) { > + scope = aml_scope("\\_SB.PCI0.ISA"); > + dev = aml_device("FDC0"); > + > + aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700"))); > + /* device present, functioning, decoding, shown in UI */ > + aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); > + > + crs = aml_resource_template(); > + aml_append(crs, > + aml_io(aml_decode16, misc->isafdc_io_base, misc->isafdc_io_base, > + 0x00, 0x04) > + ); > + aml_append(crs, > + aml_io(aml_decode16, 0x03F7, 0x03F7, 0x00, 0x01) > + ); > + aml_append(crs, aml_irq_no_flags(6)); > + aml_append(crs, aml_dma(0, false, 0, 2)); > + aml_append(dev, aml_name_decl("_CRS", crs)); > + > + aml_append(scope, dev); > + aml_append(ssdt, scope); > + } > + > if (misc->pvpanic_port) { > scope = aml_scope("\\_SB.PCI0.ISA"); > > diff --git a/hw/i386/acpi-dsdt-isa.dsl b/hw/i386/acpi-dsdt-isa.dsl > index 89caa16..061507d 100644 > --- a/hw/i386/acpi-dsdt-isa.dsl > +++ b/hw/i386/acpi-dsdt-isa.dsl > @@ -47,24 +47,6 @@ Scope(\_SB.PCI0.ISA) { > }) > } > > - Device(FDC0) { > - Name(_HID, EisaId("PNP0700")) > - Method(_STA, 0, NotSerialized) { > - Store(FDEN, Local0) > - If (LEqual(Local0, 0)) { > - Return (0x00) > - } Else { > - Return (0x0F) > - } > - } > - Name(_CRS, ResourceTemplate() { > - IO(Decode16, 0x03F2, 0x03F2, 0x00, 0x04) > - IO(Decode16, 0x03F7, 0x03F7, 0x00, 0x01) > - IRQNoFlags() { 6 } > - DMA(Compatibility, NotBusMaster, Transfer8) { 2 } > - }) > - } > - > Device(LPT) { > Name(_HID, EisaId("PNP0400")) > Method(_STA, 0, NotSerialized) { > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > index a2d84ec..81864d5 100644 > --- a/hw/i386/acpi-dsdt.dsl > +++ b/hw/i386/acpi-dsdt.dsl > @@ -81,7 +81,6 @@ DefinitionBlock ( > , 3, > CBEN, 1, // COM2 > } > - Name(FDEN, 1) > } > } > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > index 16eaca3..1645a16 100644 > --- a/hw/i386/q35-acpi-dsdt.dsl > +++ b/hw/i386/q35-acpi-dsdt.dsl > @@ -144,8 +144,7 @@ DefinitionBlock ( > Field(LPCE, AnyAcc, NoLock, Preserve) { > CAEN, 1, > CBEN, 1, > - LPEN, 1, > - FDEN, 1 > + LPEN, 1 > } > } > } >