Re: [Xen-devel] [PATCH v1 1/1] xen/arm: Map mmio-sram nodes as cached memory

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 21 Dec 2016, Julien Grall wrote:
> Hi Edgar,
> 
> On 20/12/2016 13:53, Edgar E. Iglesias wrote:
> > On Mon, Dec 19, 2016 at 04:01:00PM -0800, Stefano Stabellini wrote:
> > > On Mon, 19 Dec 2016, Julien Grall wrote:
> > > > Hi Edgar,
> > > > 
> > > > On 16/12/2016 18:04, Edgar E. Iglesias wrote:
> > > > > On Fri, Dec 16, 2016 at 04:12:00PM +0000, Julien Grall wrote:
> > > > > > On 15/12/16 11:26, Edgar E. Iglesias wrote:
> > > > > > > From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
> > > > > > > 
> > > > > > > Relax the mapping of mmio-sram nodes that do not set the
> > > > > > > no-memory-wc property to cached normal memory.
> > > > > > > 
> > > > > > > Rationale:
> > > > > > > Allthough on chip memories are relatively fast compared to
> > > > > > 
> > > > > > s/Allthough/Although/
> > > > > 
> > > > > Fixed for v2.
> > > > > 
> > > > > > 
> > > > > > > off-chip memories, large OCMs are still significantly slower
> > > > > > 
> > > > > > May I ask what does OCMs mean?
> > > > > 
> > > > > It means On Chip Memories. I can spell it out in v2.
> > > > 
> > > > Yes please. I was not able to find the acronym with a quick search
> > > > Google.
> > > > 
> > > > [...]
> > > > 
> > > > > > I consider it as the most trusted domain and has other way to mess
> > > > > > up the
> > > > > > platform. So I am fine with this relaxation for the hardware domain
> > > > > > (AKA
> > > > > > DOM0).
> > > > > > 
> > > > > > However, I have the feeling that we need this kind of relaxation for
> > > > > > many
> > > > > > devices. For instance prefetchable memory BARs for PCI would have to
> > > > > > be
> > > > > > cacheable, same for integrated graphic cards.
> > > > > 
> > > > > Yes, I agree.
> > > > > 
> > > > > > 
> > > > > > I am wondering whether for DOM0 only (*not the other guests), we
> > > > > > should
> > > > > > relax the default stage-2 attribute mapping to p2m_mmio_direct_c.
> > > > > > 
> > > > > > So we would let DOM0 in charge of the final attribute. This may
> > > > > > boost the
> > > > > > performance of memory access in DOM0.
> > > > > > 
> > > > > > Any opinions?
> > > > > 
> > > > > I think it makes sense. We had a discussiong about it a while back
> > > > > ago:
> > > > > https://lists.xenproject.org/archives/html/xen-devel/2015-05/msg02349.html
> > > > > 
> > > > > The concerns that were raised were wether there could be devices that
> > > > > could behave badly and possibly giving dom0 access to trig undefined
> > > > > or
> > > > > unpredictable behavior that could be exploited.
> > > > 
> > > > I think it would be fine for DOM0. It is already a trusted domain and
> > > > have
> > > > other way to take down the platform.
> > > > 
> > > > > 
> > > > > If dom0 accesses devices through a cache, access patterns will change.
> > > > > In theory it may trig unexpected behaviour in some device. It's hard
> > > > > to say unless talking about specific chips and implementations.
> > > > > 
> > > > > For example, given dom0 access to a DMA capable device may also
> > > > > achieve
> > > > > the same. I.e access patterns from DMA units differ from the ones
> > > > > originating from a CPU using uncached device memory. It could trig
> > > > > similar kind of errors.
> > > > 
> > > > Another example is having the device mapped with different in 2 places
> > > > with
> > > > different cacheability attributes. The data accessed would not be
> > > > coherent.
> > > > But I believe that should not lead to a security issue.
> > > > 
> > > > > 
> > > > > It would be interesting to see a concrete example of a problematic
> > > > > system.
> > > > 
> > > > I believe it would depend a lot on how the platform have been designed.
> > > > 
> > > > I think we have two choices to go forward:
> > > >         1) Relax the memory attribute on case by case. DOM0 would not 
> > > > be able
> > > > to exploit potential undefined behavior. However, this means that code
> > > > is been
> > > > added for any new device (e.g compatible string).
> > > >         2) Relax the memory attribute on every case. DOM0 would be able 
> > > > to
> > > > exploit potential undefined behavior. On the benefit side, every devices
> > > > can
> > > > be used at its full performance without any change required in Xen.
> > > > 
> > > > In the case of ACPI, we rely on DOM0 to provide the correct mapping
> > > > attribute
> > > > when asking to do the stage-2 mapping (see XENMAPSPACE_dev_mmio). Note
> > > > that
> > > > currently, the MMIO are always mapped with the attribute Device-nGnRE.
> > > > There
> > > > is plan to change that in the future. So ACPI is implementing 2).
> > > > 
> > > > Device Tree is currently implementing 1). But I would like to see the
> > > > behavior
> > > > of Xen the same no matter the firmware tables used. So I would lean
> > > > towards
> > > > 2).
> > > 
> > > That's fine by me.
> > 
> > 
> > OK, I've sent a v2 of the mmio-sram patch to avoid delays with that part.
> > 
> > But now that we seem to be in agreement with going for relaxing the default
> > S2 mappings, perhaps we should ditch the whitelist/blacklist mechanism?
> > Or does someone see a need for a blacklist?
> 
> I don't have in mind of any device requiring more tighter memory attribute
> from the hypervisor. AFAICT, it would only be necessary to do it if DOM0 was
> using wrong attribute, but that kernel would also have issue whilst running on
> baremetal.

Edgar,
I'll just wait for the patch that relaxes stage-2 mappings. I won't
apply this one.

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