[v2,0/4] tighten conditions for board-implied FDC in pc-q35-2.4+

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

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

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
>              }
>          }
>      }
>

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

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