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

["Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxx>]

On Tue, May 24, 2016 at 08:44:41PM +0100, Julien Grall wrote:
> 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?

Hi again,

Looking a little closer, the place where the generic list of matches and
attributes doesn't work well is when trying to deal with the no-memory-wc
property available only in mmio-sram nodes.

We'd really need an mmio-sram specific check in that case. Either
explicitely open coded in domain_build.c or something along the lines
f the .map method. Or did you have other ideas in mind?

Best regards,
Edgar

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