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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 24 May 2016, Julien Grall wrote:
> 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.

I agree and it has always been the intention.


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

That would work for device tree, but we still need to rely on the
hypercall for ACPI systems.

Given that it is not easy to add an additional parameter to
XENMEM_add_to_physmap_range, I think we'll have to provide a new
hypercall to allow setting attributes other than the Xen default. That
could be done in Xen 4.8 and Linux >= 4.9.

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