Re: [Qemu-devel] [PATCH v2 0/4] tighten conditions for board-implied FDC in pc-q35-2.4+

[Gabriel L. Somlo]

On Thu, May 28, 2015 at 11:50:28PM +0200, Laszlo Ersek wrote:
> >         ...
> > 
> > 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 (to both of you) for explaining. I was missing how FDEN was
being turned on/off, but now I get it.

I guess having FDC0 always present in the DSDT (with a conditional
_STA) vs. programmatically inserting it wholesale is just a matter of
aesthetics, and not worth worrying about (although being able to see
an explicit decision on whether or not to insert the entire aml blob
would arguably make it easier on inquisitive n00bs like myself) :P

Thanks again,
--Gabriel