Re: [Xen-devel] [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Julien Grall <julien.grall@xxxxxxx>]

Hi Edgar,

On 23/05/2016 16:42, Edgar E. Iglesias wrote:
> On Mon, May 23, 2016 at 04:13:53PM +0100, Julien Grall wrote:
> > On 23/05/16 15:02, Edgar E. Iglesias wrote:
> > > On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
> > > > (CC Wei Liu)
> > > >
> > > > On 23/05/16 12:56, Edgar E. Iglesias wrote:
> > > > > On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> > > > > > On 20/05/16 16:51, Edgar E. Iglesias wrote:
> > > > > > > From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
> > > > > > >
> > > > > > > This series adds support for mapping mmio-sram nodes into dom0
> > > > > > > as MEMORY, cached and with RWX perms.
> > > > > > Can you explain why you chose to map those nodes as MEMORY, cached and with
> > > > > > RWX perms?
> > > > > My understanding is that these mmio-sram nodes are allowed to be treated as
> > > > > Normal memory by the guest OS.
> > > > > Guests could potentially do any kind of memory like operations on them.
> > > > >
> > > > > In our specific case, dom0 won't execute code from these regions but
> > > > > Linux/dom0 ends up using standard memcpy/memset/x functions (not
> > > > > memcpy_fromio and friends) on the regions.
> > > > I looked at the generic sram driver in Linux (drivers/misc/sram.c) which
> > > > actually use memcpy_{to,from}io. So how you driver differs from the generic
> > > > one? What the SRAM will contain?
> > > We intend to use that same driver to map the memory but mmio-sram
> > > nodes allow you to assign the regions to other device-drivers.
> > >
> > > Some examples:
> > > Documentation/devicetree/bindings/crypto/marvell-cesa.txt
> > > arch/arm/boot/dts/orion5x.dtsi
> > > drivers/crypto/mv_cesa.c
> > >
> > > The cover letter for the sram driver had an example aswell allthough
> > > the function names have changed since (it's of_gen_pool_get now):
> > > https://lwn.net/Articles/537728/
> > >
> > > Nothing explicitely says that the regions can be assumed to be mapped
> > > as Normal memory, but on Linux they do get mapped as Mormal WC mem
> > > (unless the no-memory-wc prop is set on the node).
> > > The marvell-cesa example also uses plain memset on the sram.
> > I am a bit confused with this example. From my understanding of
> > mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area (see
> > gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).
> >
> > However, memcpy_{from,to}io should be used when dealing with MMIO (the field
> > sram has the __iomem attribute). See the commit 0f3304dc from Russel King
> > related to marvell/cesa.
>
> Yeah, I'm started to get confused too. Maybe they just forgot the memset
> in drivers/crypto/mv_cesa.c.
>
> There are other examples though, that don't do fromio/toio at all.
> Documentation/devicetree/bindings/media/coda.txt
> drivers/media/platform/coda/coda-common.c
>
> Allthough ofcourse, these could also be wrong. Maybe I've missunderstood how
> mmio-sram is supposed to be used.
I have talked about the memory attribute around me and the consensus is 
we should use the most relaxed mode that does not have any security 
implication or undefined behavior for a given device.
For SRAM it would be normal memory uncached (?) when the property 
"no-memory-wc" is not present, else TBD.
I suspect we would have to relax more MMIOs in the future. Rather than 
providing a function to map, the code is very similar except the memory 
attribute, I suggest to provide a list of compatible with the memory 
attribute to use.
All the children node would inherit the memory attribute of the parent.

What do you think?

Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel