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

[Laszlo Ersek]

On 05/29/15 01:53, Gabriel L. Somlo wrote:
> 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

Well, it seems like pvpanic for example follows your preferred approach,
so as far as I'm concerned, feel free to champion this cause.
Preferably, *after* this series goes in. ;)

Cheers
Laszlo