[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: |
Thu, 28 May 2015 23:50:28 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
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
> }
> }
> }
>
- [Qemu-devel] [PATCH v2 0/4] tighten conditions for board-implied FDC in pc-q35-2.4+, Laszlo Ersek, 2015/05/28
- [Qemu-devel] [PATCH v2 1/4] i386/pc: pc_basic_device_init(): delegate FDC creation request, Laszlo Ersek, 2015/05/28
- [Qemu-devel] [PATCH v2 2/4] i386/pc: '-drive if=floppy' should imply a board-default FDC, Laszlo Ersek, 2015/05/28
- [Qemu-devel] [PATCH v2 3/4] i386/pc_q35: don't insist on board FDC if there's no default floppy, Laszlo Ersek, 2015/05/28
- [Qemu-devel] [PATCH v2 4/4] i386: drop FDC in pc-q35-2.4+ if neither it nor floppy drives are wanted, Laszlo Ersek, 2015/05/28
- Re: [Qemu-devel] [PATCH v2 0/4] tighten conditions for board-implied FDC in pc-q35-2.4+, Gabriel L. Somlo, 2015/05/28
- Re: [Qemu-devel] [PATCH v2 0/4] tighten conditions for board-implied FDC in pc-q35-2.4+,
Laszlo Ersek <=
- Re: [Qemu-devel] [PATCH v2 0/4] tighten conditions for board-implied FDC in pc-q35-2.4+, Markus Armbruster, 2015/05/29