[for-4.7] Request input on XENMAPSPACE_dev_mmio

[for-4.7] Request input on XENMAPSPACE_dev_mmio

[Julien Grall]

Hi all,

Sorry for noticing this problem late in the release process.

The mapping space XENMAPSPACE_dev_mmio has been introduced recently 
(will be present in Xen 4.7) to let dom0 map device MMIO regions when 
ACPI is in-use on ARM platform.

Xen ARM will map those regions in the stage-2 page table using the 
memory attribute Device_nGnRE (i.e non-Gathering, non-Reordering, 
no-unaligned access).

The final memory attribute is a combination of stage-1 (handled by the 
kernel) and stage-2 attributes. It will always result to a restrictive 
memory attribute (see D4.4.3 in ARM DDI 0487A.i).

Device_nGnRE is one of the most restrictive memory attribute, whilst it 
fits in lot of case, the performance are not great on device such as 
graphic cards, and it makes impossible to access those regions with 
unaligned access (a lot of Linux drivers uses memcpy which does 
unaligned access).

Unfortunately it is not possible to find a weaker memory attribute that 
would fit for everyone. For instance, we would need to map static RAM 
with normal memory attribute to allow speculation and unaligned access.

However, the same normal memory could not be used for MMIOs having 
side-effect (such as an UART) because the processor would be allowed to 
fetch additional memory locations.

For more details about the different memory attributes see B2.8 in ARM 
DDI 0487A.i.

In the case of ACPI, only DOM0 has access to the full description of the 
device and know the memory attribute to use. So we need to expand
XENMAPSPACE_dev_mmio to provide the memory attribute (this could be done 
using the top bits of 'space').

For ARM we would need at least the following attributes:
   - normal uncached memory: for write-combine on SRAM or video RAM
   - Device_nGnRE: non-gathering and non-reordering
   - Device_GRE: gathering and redordering

It might be worth to also consider "normal cache memory".

Xen 4.7 will be release very soon (~ couple of weeks), so we have few 
solutions:
     1) Extend XENMAPSPACE_dev_mmio for Xen 4.7
     2) Wait for Xen 4.8 to fix it: this may require to introduce a new 
space to be backward compatible.
     3) Revert XENMAPSPACE_dev_mmio for Xen 4.7 and re-introduce it 
properly on Xen 4.8: It is only used by ACPI on ARM which is a tech preview.

I would lean toward the solution 1) to avoid delaying ACPI support for 
ARM and avoid introducing an sub-hypercall which does not fit for our usage.

I think the space could be extend in a lightweight version for Xen 4.7 
by introducing only one memory attribute and warn on any other value.

Do you have any opinions on the way to fix it? I can provide a patch as 
soon as possible.

Regards,

-- 
Julien Grall

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

Re: [for-4.7] Request input on XENMAPSPACE_dev_mmio

[Jan Beulich]

>>> On 24.05.16 at 15:24, <julien.grall@arm.com> wrote:
> For ARM we would need at least the following attributes:
>    - normal uncached memory: for write-combine on SRAM or video RAM
>    - Device_nGnRE: non-gathering and non-reordering
>    - Device_GRE: gathering and redordering
> 
> It might be worth to also consider "normal cache memory".
> 
> Xen 4.7 will be release very soon (~ couple of weeks), so we have few 
> solutions:
>      1) Extend XENMAPSPACE_dev_mmio for Xen 4.7
>      2) Wait for Xen 4.8 to fix it: this may require to introduce a new 
> space to be backward compatible.
>      3) Revert XENMAPSPACE_dev_mmio for Xen 4.7 and re-introduce it 
> properly on Xen 4.8: It is only used by ACPI on ARM which is a tech preview.
> 
> I would lean toward the solution 1) to avoid delaying ACPI support for 
> ARM and avoid introducing an sub-hypercall which does not fit for our usage.

Option 2 is clearly worse. I view option 3 as a possibility, but only if
option 1 turns out too intrusive or some such.

However, using the top bits of space doesn't look a very nice way
to address this. How about defining one attribute which would
always get used by XENMEM_add_to_physmap (perhaps the one
that gets used it is right now), and supporting a wider range only
through XENMEM_add_to_physmap_batch? Background being that
the latter has an unused (currently ignored) field, which you could
utilize: foreign_domid. Of course that would then need to be
renamed in a backward compatible way (to something more generic,
e.g. "aux"), and we should try to remember to avoid the same
mistake that was made when that sub-op was added and again
when XENMAPSPACE_dev_mmio got introduced: New operations
should not ignore fields, so they can get a meaning assigned later
on.

> I think the space could be extend in a lightweight version for Xen 4.7 
> by introducing only one memory attribute and warn on any other value.

Not sure what lightweight version you think of here, or how you
would mean to make a hypercall "warn" - it can only return success
or an error.

Jan


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

Re: [for-4.7] Request input on XENMAPSPACE_dev_mmio

[Wei Liu]

On Tue, May 24, 2016 at 02:24:22PM +0100, Julien Grall wrote:
> Hi all,
> 
> Sorry for noticing this problem late in the release process.
> 
> The mapping space XENMAPSPACE_dev_mmio has been introduced recently (will be
> present in Xen 4.7) to let dom0 map device MMIO regions when ACPI is in-use
> on ARM platform.
> 
> Xen ARM will map those regions in the stage-2 page table using the memory
> attribute Device_nGnRE (i.e non-Gathering, non-Reordering, no-unaligned
> access).
> 
> The final memory attribute is a combination of stage-1 (handled by the
> kernel) and stage-2 attributes. It will always result to a restrictive
> memory attribute (see D4.4.3 in ARM DDI 0487A.i).
> 
> Device_nGnRE is one of the most restrictive memory attribute, whilst it fits
> in lot of case, the performance are not great on device such as graphic
> cards, and it makes impossible to access those regions with unaligned access
> (a lot of Linux drivers uses memcpy which does unaligned access).
> 
> Unfortunately it is not possible to find a weaker memory attribute that
> would fit for everyone. For instance, we would need to map static RAM with
> normal memory attribute to allow speculation and unaligned access.
> 
> However, the same normal memory could not be used for MMIOs having
> side-effect (such as an UART) because the processor would be allowed to
> fetch additional memory locations.
> 
> For more details about the different memory attributes see B2.8 in ARM DDI
> 0487A.i.
> 
> In the case of ACPI, only DOM0 has access to the full description of the
> device and know the memory attribute to use. So we need to expand
> XENMAPSPACE_dev_mmio to provide the memory attribute (this could be done
> using the top bits of 'space').
> 
> For ARM we would need at least the following attributes:
>   - normal uncached memory: for write-combine on SRAM or video RAM
>   - Device_nGnRE: non-gathering and non-reordering
>   - Device_GRE: gathering and redordering
> 
> It might be worth to also consider "normal cache memory".
> 
> Xen 4.7 will be release very soon (~ couple of weeks), so we have few
> solutions:
>     1) Extend XENMAPSPACE_dev_mmio for Xen 4.7
>     2) Wait for Xen 4.8 to fix it: this may require to introduce a new space
> to be backward compatible.
>     3) Revert XENMAPSPACE_dev_mmio for Xen 4.7 and re-introduce it properly
> on Xen 4.8: It is only used by ACPI on ARM which is a tech preview.
> 
> I would lean toward the solution 1) to avoid delaying ACPI support for ARM
> and avoid introducing an sub-hypercall which does not fit for our usage.
> 

My preference is 1>3>2, fwiw.

After looking at the code, my gut feeling is that 1 is not going to be
overly intrusive -- there seems to be only a handful of function in Xen
on ARM to handle MMIO mapping.

> I think the space could be extend in a lightweight version for Xen 4.7 by
> introducing only one memory attribute and warn on any other value.
> 

We should just reject the values that are not supported yet imho.

Wei.

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

Re: [for-4.7] Request input on XENMAPSPACE_dev_mmio

[Julien Grall]

Hi Jan,

On 24/05/16 14:57, Jan Beulich wrote:
>>>> On 24.05.16 at 15:24, <julien.grall@arm.com> wrote:
>> For ARM we would need at least the following attributes:
>>     - normal uncached memory: for write-combine on SRAM or video RAM
>>     - Device_nGnRE: non-gathering and non-reordering
>>     - Device_GRE: gathering and redordering
>>
>> It might be worth to also consider "normal cache memory".
>>
>> Xen 4.7 will be release very soon (~ couple of weeks), so we have few
>> solutions:
>>       1) Extend XENMAPSPACE_dev_mmio for Xen 4.7
>>       2) Wait for Xen 4.8 to fix it: this may require to introduce a new
>> space to be backward compatible.
>>       3) Revert XENMAPSPACE_dev_mmio for Xen 4.7 and re-introduce it
>> properly on Xen 4.8: It is only used by ACPI on ARM which is a tech preview.
>>
>> I would lean toward the solution 1) to avoid delaying ACPI support for
>> ARM and avoid introducing an sub-hypercall which does not fit for our usage.
>
> Option 2 is clearly worse. I view option 3 as a possibility, but only if
> option 1 turns out too intrusive or some such.
>
> However, using the top bits of space doesn't look a very nice way
> to address this. How about defining one attribute which would
> always get used by XENMEM_add_to_physmap (perhaps the one
> that gets used it is right now), and supporting a wider range only
> through XENMEM_add_to_physmap_batch? Background being that
> the latter has an unused (currently ignored) field, which you could
> utilize: foreign_domid. Of course that would then need to be
> renamed in a backward compatible way (to something more generic,
> e.g. "aux"), and we should try to remember to avoid the same
> mistake that was made when that sub-op was added and again
> when XENMAPSPACE_dev_mmio got introduced: New operations
> should not ignore fields, so they can get a meaning assigned later
> on.

That is a good idea. For Xen 4.7, I am suggesting to:
   - Document the behavior of XENMEM_add_to_physmap for ARM. I.e it will 
always use Device_nGnRE (we could use a less weaker one but I am not 
confident enough for a such change before the release).
   - Check the foreign_id is always with XEMEM_add_to_physmap_batch. The 
memory attribute associated will be the same as XENMEM_add_to_physmap.

The number of memory attribute supported will be extend after the 
release for Xen 4.8.

>> I think the space could be extend in a lightweight version for Xen 4.7
>> by introducing only one memory attribute and warn on any other value.
>
> Not sure what lightweight version you think of here, or how you
> would mean to make a hypercall "warn" - it can only return success
> or an error.

Let say we only support one kind of memory attribute for Xen 4.7. When 
we will introduce new one, Linux may use them and therefore the mapping 
will fail.

We can either fix by letting Linux try different memory attribute. Or 
use a default one in Xen.

Regards,

-- 
Julien Grall

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