Re: [Qemu-devel] [PATCH v5 0/5] add ACPI node for fw_cfg on pc and arm

[Gabriel L. Somlo]

Couple of items:

1. Ping ? :)

2. Thank you markmb for your R-b !

3. If anyone's had a chance to look over the corresponding guest-side
   kernel sysfs driver which utilizes this, you will have noticed I'm
   automatically initializing the driver based on DeviceTree or ACPI
   on ARM and x86, and that there's also the option of passing in
   command line parameters for the other architectures on which fw_cfg
   is available (sun4* and ppc/mac).

   The issue I'm wondering about is that, while architectures where
   fw_cfg registers show up as IO ports (x86 and sun4u) have the same
   register_offset:size values (0:2 for control, 1:1 for data), MMIO
   on everything *other* than ARM is different:

        - ARM has 8:2 for control, and 0:8 for data

        - sun4m and ppc/mac both have 0:2 for control, and 2:1 for data

   Right now, neither DeviceTree nor ACPI specify register offsets
   within the MMIO or PortIO area they associate with fw_cfg:

        - ACPI has (in _CRS) either:

            IO (Decode16,
                0x0510,             // Range Minimum
                0x0510,             // Range Maximum
                0x01,               // Alignment
                0x02,               // Length
                )

          or:

            Memory32Fixed (ReadWrite,
                           0x09020000,  // Address Base
                           0x0000000a,  // Address Length
                          )

        - DT does something along this lines:

            {
                    #size-cells = <0x2>;
                    #address-cells = <0x2>;

                    address@hidden {
                            compatible = "qemu,fw-cfg-mmio";
                            reg = <0x0 0x9020000 0x0 0xa>;
                    };
            };

  So in the guest-side kernel sysfs driver initialization routine,
  I need to assume x86 (and, respectively, ARM) register offset:size
  values unless explicitly provided on the command line.

  Are we likely to EVER try to supply defaults via DT (or ACPI) for
  any other architectures besides ARM and x86 ? If so, is there a way
  to additionally provide offsets (and sizes), besides just the
  overall range ?

  If we are NOT planning to ever do DT or ACPI outside x86 and ARM,
  then nevermind I said anything :)

Thanks much,
--Gabriel 

On Fri, Nov 13, 2015 at 09:57:13PM -0500, Gabriel L. Somlo wrote:
> New since v4:
> 
>       - rebased on top of Marc's DMA series
>       - drop machine compat dependency for insertion into x86/ssdt
>         (patch 3/5), following agreement between Igor and Eduardo
>       - [mm]io register range now covers DMA register as well, if
>         available.
>       - s/bios/firmware in doc file updates
> 
> Thanks,
>   --Gabriel
> 
> >New since v3:
> >
> >     - rebased to work on top of 87e896ab (introducing pc-*-25 classes),
> >       inserting fw_cfg acpi node only for machines >= 2.5.
> >
> >     - reintroduce _STA with value 0x0B (bit 2 for u/i visibility turned
> >       off to avoid Windows complaining -- thanks Igor for catching that!)
> >
> >If there's any other feedback besides questions regarding the
> >appropriateness of "QEMU0002" as the value of _HID, please don't hesitate!
> >
> >>New since v2:
> >>
> >>    - pc/i386 node in ssdt only on machine types *newer* than 2.4
> >>      (as suggested by Eduardo)
> >>
> >>I appreciate any further comments and reviews. Hopefully we can make
> >>this palatable for upstream, modulo the lingering concerns about whether
> >>"QEMU0002" is ok to use as the value of _HID, which I'll hopefully get
> >>sorted out with the kernel crew...
> >>
> >>>New since v1:
> >>>
> >>>   - expose control register size (suggested by Marc Marí)
> >>>
> >>>   - leaving out _UID and _STA fields (thanks Shannon & Igor)
> >>>
> >>>   - using "QEMU0002" as the value of _HID (thanks Michael)
> >>>
> >>>   - added documentation blurb to docs/specs/fw_cfg.txt
> >>>     (mainly to record usage of the "QEMU0002" string with fw_cfg).
> >>>
> >>>> This series adds a fw_cfg device node to the SSDT (on pc), or to the
> >>>> DSDT (on arm).
> >>>>
> >>>>  - Patch 1/3 moves (and renames) the BIOS_CFG_IOPORT (0x510)
> >>>>    define from pc.c to pc.h, so that it could be used from
> >>>>    acpi-build.c in patch 2/3.
> >>>> 
> >>>>  - Patch 2/3 adds a fw_cfg node to the pc SSDT.
> >>>> 
> >>>>  - Patch 3/3 adds a fw_cfg node to the arm DSDT.
> >>>>
> >>>> I made up some names - "FWCF" for the node name, and "FWCF0001"
> >>>> for _HID; no idea whether that's appropriate, or how else I should
> >>>> figure out what to use instead...
> >>>>
> >>>> Also, using scope "\\_SB", based on where fw_cfg shows up in the
> >>>> output of "info qtree". Again, if that's wrong, please point me in
> >>>> the right direction.
> >>>>
> >>>> Re. 3/3 (also mentioned after the commit blurb in the patch itself),
> >>>> I noticed none of the other DSDT entries contain a _STA field, wondering
> >>>> why it would (not) make sense to include that, same as on the PC.
> Gabriel L. Somlo (5):
>   fw_cfg: expose control register size in fw_cfg.h
>   pc: fw_cfg: move ioport base constant to pc.h
>   acpi: pc: add fw_cfg device node to ssdt
>   acpi: arm: add fw_cfg device node to dsdt
>   fw_cfg: document ACPI device node information
> 
>  docs/specs/fw_cfg.txt     |  9 +++++++++
>  hw/arm/virt-acpi-build.c  | 15 +++++++++++++++
>  hw/i386/acpi-build.c      | 29 +++++++++++++++++++++++++++++
>  hw/i386/pc.c              |  5 ++---
>  hw/nvram/fw_cfg.c         |  4 +++-
>  include/hw/i386/pc.h      |  2 ++
>  include/hw/nvram/fw_cfg.h |  3 +++
>  7 files changed, 63 insertions(+), 4 deletions(-)
> 
> -- 
> 2.4.3
>