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

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?

> > +
> > +Target address goes up and transfer length goes down as the
> > transfer +happens, so after a successful transfer the length
> > register is zero +and the address register points right after the
> > memory block written. +
> > +If a partial transfer happened before an error occured the address
> > and +length registers indicate how much data has been transfered
> > +successfully.
> > +
> >  Required properties:
> >  
> >  - compatible: "qemu,fw-cfg-mmio".
> > @@ -56,6 +91,7 @@ Required properties:
> >  - reg: the MMIO region used by the device.
> >    * Bytes 0x0 to 0x7 cover the data register.
> >    * Bytes 0x8 to 0x9 cover the selector register.
> > +  * From revision 2: Bytes 0xa to 0x11 cover the DMA address
> > register.
> 
> I think we should naturally align the register, i.e we can reserve
> bytes 0xa - 0xf, and then put the DMA register at 0x10.
> 
> >    * Further registers may be appended to the region in case of
> > future interface revisions / feature bits.
> >  
> > -- 
> > 2.4.3
> > 
> 

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/