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

[Laszlo Ersek <lersek@redhat.com>]

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