Date: Thu, 6 Aug 2015 17:13:31 +0200
From: Marc =?UTF-8?B?TWFyw60=?= <markmb@redhat.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: Andrew Jones <drjones@redhat.com>, linux-kernel@vger.kernel.org,
        Stefan Hajnoczi <stefanha@gmail.com>,
        "Kevin O'Connor" <kevin@koconnor.net>,
        Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [PATCH] QEMU fw_cfg DMA interface documentation
Message-ID: <20150806171331.02e14431@markmb_rh>
In-Reply-To: <55C37558.1080109@redhat.com>
References: <1438858816-29385-1-git-send-email-markmb@redhat.com>
	<1438858987-29566-1-git-send-email-markmb@redhat.com>
	<20150806140849.GD3083@hawk.localdomain>
	<20150806161918.160afd5e@markmb_rh>
	<20150806142822.GE3083@hawk.localdomain>
	<55C37558.1080109@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5223
Lines: 123

On Thu, 6 Aug 2015 16:55:20 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 08/06/15 16:28, Andrew Jones wrote:
> > On Thu, Aug 06, 2015 at 04:19:18PM +0200, Marc Marí wrote:
> >> On Thu, 6 Aug 2015 16:08:49 +0200
> >> Andrew Jones <drjones@redhat.com> wrote:
> >>
> >>> On Thu, Aug 06, 2015 at 01:03:07PM +0200, Marc Marí wrote:
> >>>> Add fw_cfg DMA interface specfication in the fw_cfg
> >>>> documentation.
> >>>>
> >>>> Signed-off-by: Marc Marí <markmb@redhat.com>
> >>>> ---
> >>>>  Documentation/devicetree/bindings/arm/fw-cfg.txt | 36
> >>>> ++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/arm/fw-cfg.txt
> >>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt index
> >>>> 953fb64..c880eec 100644 ---
> >>>> a/Documentation/devicetree/bindings/arm/fw-cfg.txt +++
> >>>> b/Documentation/devicetree/bindings/arm/fw-cfg.txt @@ -49,6
> >>>> +49,41 @@ The guest kernel is not expected to use these registers
> >>>> (although it is certainly allowed to); the device tree bindings
> >>>> are documented here because this is where device tree bindings
> >>>> reside in general. +Starting from revision 2, a DMA interface
> >>>> has also been added. This can be used +through a write-only,
> >>>> 64-bit wide address register. +
> >>>> +In this register, a pointer to a FWCfgDmaAccess structure can be
> >>>> written, in +big endian mode. This is the format of the
> >>>> FWCfgDmaAccess structure:
> >>> s/mode/format/
> >>>> +
> >>>> +typedef struct FWCfgDmaAccess {
> >>>> +    uint64_t address;
> >>>> +    uint32_t length;
> >>>> +    uint32_t control;
> >>>> +} FWCfgDmaAccess;
> >>>> +
> >>>> +Once the address to this structure has been written, an DMA
> >>>> operation is +started. If the "control" field has value 2, a read
> >>>> operation will be performed. +"length" bytes for the current
> >>>> selector and offset will be mapped into the +address specified by
> >>>> the "address" field. +
> >>>> +If the field "address" has value 0, the read is considered a
> >>>> skip, and +the data is not copied anywhere, but the offset is
> >>>> still incremented.
> >>>
> >>> So we can't DMA to address == 0? That might not generally be a
> >>> good idea, but why limit ourselves? Can't we add another control
> >>> input for skip instead? Actually, what inputs are accepted now?
> >>> READ == 2, WRITE? ??
> >>>
> >>
> >> Write was already disabled for PIO:
> >>
> >> static void fw_cfg_write(FWCfgState *s, uint8_t value)
> >> {
> >>     /* nothing, write support removed in QEMU v2.4+ */
> >> }
> >>
> >>>> +
> >>>> +To check result, read the control register:
> >>>> +   error bit set     ->  something went wrong.
> >>> echo Stefan's which bit question, and also what types of errors?
> >>> If there are many, then how about an error bit, plus field of
> >>> bits for an error code?
> >>>
> >>
> >> Bit 0. At this moment the only error possible is DMA mapping
> >> failure. But its true that it might be useful to have some bits in
> >> the control field or another field to indicate the type of error,
> >> in case of future extensions.
> >>
> >>>> +   all bits cleared  ->  transfer finished successfully.
> >>>> +   otherwise         ->  transfer still in progress (doesn't
> >>>> happen
> >>>> +                         today due to implementation not being
> >>>> async,
> >>>> +                         but may in the future).
> >>> I don't think we need to point out that this isn't implemented
> >>> yet, but may be in the future. If that's how it may work, then
> >>> let's just document it. And why not specify an in-progress bit?
> >>
> >> Is this a feature we will want?
> > 
> > I don't know. Need firmware person like Laszlo to give an opinion.
> > Maybe he'd want OVMF/AAVMF to be able to output progress while
> > transferring, to keep users more patient?
> 
> I don't yet know how to use this interface from the firmware.
> 
> FWIW the library interface to fw_cfg that we use in OVMF & AAVMF is
> synchronous. (It doesn't make sense to do "something else" until the
> fw_cfg transfer completes "in the background".)
> 
> So I imagined the new interface would behave in one of the following
> ways: (1) I perform the "final" MMIO register write, and bam, by the
> time the next instruction is executed, the target memory area is
> populated. (2) Or, I perform the same final MMIO register write, and
> then busy-loop on reading some other status register, until it tells
> me that the transfer is done, or there has been an error.
> 

It's (2).

If you have library functions such as "fw_cfg_read", just changing
them should leave everything working. You may want to look at the x86
guest (SeaBIOS) to check the few changes necessary:

http://www.seabios.org/pipermail/seabios/2015-August/009605.html

But it's important to note that the host part (QEMU) is implemented
using memcpy. When you implement this interface, you may want to
increase the chunk size.

Thanks
Marc
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/