[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 0/4] tighten conditions for board-implied FDC

From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH v2 0/4] tighten conditions for board-implied FDC in pc-q35-2.4+
Date: Fri, 29 May 2015 02:27:19 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0

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. ;)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]