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

[Julien Grall <julien.grall@arm.com>]

(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@xilinx.com>
>>>
>>> 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@lists.xen.org
http://lists.xen.org/xen-devel