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

(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 saw issues with memset doing cache operations on DEVICE memory in
> the past (external data aborts). I can't find the text in the ARM ARM
> regarding this at the moment but IIRC, dc ops on device mem are not
> allowed.
The ARM ARM (D3-1632 in ARM DDI 0487A.i) states "that the effects of the 
cache maintenance instructions can apply regardless of:
Whether the address accessed:
        * Is Normal memory or Device memory.
        * Has the Cacheable attribute or the Non-cacheable attribute.
"

So I am not sure why you would get an external data aborts when 
executing dc ops on device memory.
> We could also see alignment problems, as the alignment checks for
> ARMv8 differ somewhat between normal memory and device memory.
>
> A third reason is performance. The rules for speculative accesses
> and caching are different between device and normal memory.
>
> So I opted for the most permissive attributes thinking that dom0 can
> apply further restrictions if it needs to do so.
>
>
> > > Dom0 can then choose to further restrict these mappings if needed.
> > > We only look at the outer mmio-sram region. The sub-area nodes that
> > > describe allocations within the mmio-sram region are only meaningful
> > > to the guest AFAICT.
> > >
> > > In an attempt to avoid growing the already fairly large domain_build.c
> > > file, I've tried to implement a distributed way to deal with these kind
> > > of special/custom mapping needs. These can live outside of domain_build.c
> > > and are registerd by means of a .map method in the device_spec.
> > >
> > > If this approach is not good, I'm happy to bin it and try something else.
> > We will have a similar problem when using ACPI for DOM0 or mapping a such
> > MMIO to the guest. The hypercalls XENMAPSPACE_dev_mmio and
> > XEN_DOMCTL_memory_mapping do not provide enough information to know the
> > attribute to be used for mapping.
> >
> > MMIO are always mapped in Stage-2 with Device_nGnRE, which is quite
> > restrictive. This would also impact any MMIO regions, such as the video RAM
> > buffer, that could be mapped write-combine.
> >
> > After reading the ARM ARM (B2.8.2 ARM DII 0486.i), I think we could relax
> > the stage-2 mapping by using Device_GRE for all the device MMIOs but the
> > GIC.
> >
> > We have to keep the GIC MMIO with the most restrictive memory attribute to
> > avoid potential side-effect when Xen is switching between multiple vCPUs.
> I see, that makes sense.
>
>
> > All the other devices will be exclusive to a specific guest, so the guest
> > can handle the device the way it wants. This may require some extra-care
> > when reassigning the device to another domain.
> >
> > Edgar, would Device_GRE be fine for you?
> Sorry, but I don't think so. We can live with performance impacts but it's
> harder with the external data aborts and potential alignment checks.
>
>
> > Note, that XENMAPSPACE_dev_mmio has been introduced in Xen 4.7 (which is due
> > in a couple of weeks) and part of the stable ABI. So if it is not possible
> > to relax the memory attribute, it might be worth to think fixing/reverting
> > the hypercall for 4.7. Otherwise we would have to introduce a new one in the
> > next release.
> Yes, maybe we could add something along the lines of the pgprot arg
> that Linux/arm64 has in it's __ioremap call. Even if we only support
> PROT_DEVICE_nGnRE (or GRE) in 4.7 at the least we can reuse the ABI
> to add more modes in 4.8?
I will bring it on a separate subject with the REST of the maintainers 
when we find out what are the possible memory attributes.
Depending on the result, this might be considered as a blocker as I do 
not think we should avoid to  introduce a new hypercall 
(XENMAPSPACE_dev_mmio) which is known to not fit for every case.
Regards,

--
Julien Grall

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