Re: [Xen-devel] [PATCH RFC 25/35] arm: acpi add helper functions to map memory regions

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Stefano,

On 06/02/2015 22:12, Stefano Stabellini wrote:
> On Fri, 6 Feb 2015, Julien Grall wrote:
> > On 06/02/2015 00:21, Stefano Stabellini wrote:
> > > On Thu, 5 Feb 2015, Julien Grall wrote:
> > > > Hi parth,
> > > >
> > > > Title: this is not acpi specific.
> > > >
> > > > On 04/02/2015 14:02, parth.dixit@xxxxxxxxxx wrote:
> > > > > From: Parth Dixit <parth.dixit@xxxxxxxxxx>
> > > > >
> > > > > For passing ACPI tables to dom0, UEFI memory needs to be mapped
> > > > > by xen in dom0 address space. This patch adds helper functions for
> > > > > mapping.
> > > > I believe that this is not ACPI/RAM specific. Any cached MMIO regions will
> > > > have same issue.
> > > >
> > > > This because Device memory is too strong and disallow unaligned access.
> > > >
> > > > > Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
> > > > > ---
> > > > >     xen/arch/arm/p2m.c        | 24 ++++++++++++++++++++++++
> > > > >     xen/include/asm-arm/p2m.h | 10 ++++++++++
> > > > >     2 files changed, 34 insertions(+)
> > > > >
> > > > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > > > index 8809f5a..5593a91 100644
> > > > > --- a/xen/arch/arm/p2m.c
> > > > > +++ b/xen/arch/arm/p2m.c
> > > > > @@ -943,6 +943,30 @@ int p2m_populate_ram(struct domain *d,
> > > > >                                  0, MATTR_MEM, p2m_ram_rw);
> > > > >     }
> > > > >
> > > > > +int map_ram_regions(struct domain *d,
> > > > > +                     unsigned long start_gfn,
> > > > > +                     unsigned long nr,
> > > > > +                     unsigned long mfn)
> > > > I don't like the name of the function. It gives the impression that we map
> > > > any
> > > > RAM region to the guest via this function.
> > > >
> > > > Which is obviously wrong and should never be done.
> > > map_mem_regions?
> > It's still unclear, what mem mean? is an MMIO region? Is it cached, uncached?
> > Is it read-only/read-write.
> >
> > We already have a function map_mmio_regions. Maybe it would make sense to
> > extend it to give more control about the mapping (cached/uncached, read-only,
> > read-write,....). But this function is common with x86. So I'm not sure about
> > what to do.
> Usually mmio refers to device memory and not just from the caching
> attributes point of view.
I didn't find anything which state ACPI is always living in the RAM. It 
could also be in the ROM.
> I would rather not use map_mmio_regions for
> something that doesn't belong to a device.
The current MMIO code assume that the memory will always be non-cached 
(MATTR_DEVICE). This will lead to various issue in the guest when it's 
trying to access cached BARs.
It would be good to have a common helper handling this case because we 
will have to handle it for PCI sooner or later.
> That said, it is difficult to come up with a good name.
Maybe we could introduce a function map_region which the following prototype

map_region(strut domain *d, unsigned long start_gfn,
           unsigned long nr, unsigned long mfn,
           int mattr, p2m_type_t type)

We could also drop add new types (either real or virtual) to 
differentiate the use cases. This is because we can infer the mattr 
(MATTR_DEVICE/MATTR_MEM).
At the same time, it would clean up a bit the current code by dropping 
one parameter.
Regards,

--
Julien Grall

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