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

[Gabriel L. Somlo]

On Mon, Nov 23, 2015 at 08:46:33PM +0100, Laszlo Ersek wrote:
> On 11/23/15 20:31, Gabriel L. Somlo wrote:
> > 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 ?
> 
> I guess this is where the bindings docs would provide the specification...

OK, but is there a way to *functionally* specify register offsets, in
a way that can automagically get translated into a set of

        ( IORESOURCE_IO | IORESOURCE_MEM ) + IORESOURCE_REG + ...

? Right now we get _IO or _MEM only, and have to assume the register
offsets based on the architecture. I tried to figure this out on my
own, but nothing obvious stands out either in the way DT nodes are
documented, nor in the way ACPI nodes are written in several of the
machines I acpidump-ed to look for clues [*]. Maybe not too many devices
have this much variability across the platforms they're deployed on :)

[*] In ACPI there's "Offset", but that's for accessing fields within an
    operation range, and there's "Register" which seems promising:

    replace IO(...) or Memory32Fixed(...) with a set of Register(...)
    statements in _CRS, but not immediately clear how one would
    specify which one is Data vs Ctrl (order ?), and whether there's
    an equivalent way to specify this via DT...


I guess there's always something like

#if defined ARCH_X86

#define CTRL_OFF 0x00
#define DATA_OFF 0x01

#elif defined ARCH_ARM

#define CTRL_OFF 0x08
#define DATA_OFF 0x00

#else ...

Although I haven't seen a lot of that done anywhere in the kernel, so
it might be frowned upon... :)


Thanks,
--Gabriel


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