Re: [Xen-devel] [PATCH v1 1/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c

["Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxx>]

On Thu, Jan 26, 2017 at 12:58:52PM +0000, Julien Grall wrote:
> Hi Edgar,

Hi Julien,

> 
> On 26/01/2017 12:52, Edgar E. Iglesias wrote:
> >On Thu, Jan 19, 2017 at 12:40:45PM +0000, Julien Grall wrote:
> >>
> >>On 10/01/2017 11:37, Edgar E. Iglesias wrote:
> >>>From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
> >>>
> >>>Relax the hardware domains mapping attributes to p2m_mmio_direct_c.
> >>>This will allow the hardware domain to fully control the
> >>>attribtues via its S1 mappings.
> >>
> >>s/attribtues/attributes/
> >
> >Fixed for v2.
> >
> >
> >>
> >>I would like some rationale in the commit message to explain why it is fine
> >>to do this relaxation (e.g the hardware domain is a trusted domain).
> >
> >I've added the following for v2:
> >    Since the hardware domain is a trusted domain, we extend the
> >    trust to include making final decisions on what attributes to
> >    use when mapping memory regions.
> >
> >    For device-tree configured hardware domains, this patch relaxes
> 
> I would drop the "For device-tree configured hardware domains" as you will
> also fix ACPI. The rest looks good to me.

Sorry, saw this a bit late but the series has one patch fixing DT and
another fixing ACPI. If you prefer I can squash them in a v3 or perhaps
Stefano can squash it as he commits them.


> 
> >    the hardware domains mapping attributes to p2m_mmio_direct_c.
> >    This will allow the hardware domain to control the attributes
> >    via its S1 mappings.
> >
> >>
> >>A such relaxation would probably be necessary for the ACPI case too (see
> >>map_dev_mmio_region).
> >
> >I don't have testcases for ACPI but I'll try to fix it.
> >
> >Please correct me if I'm wrong. IIUC, when using ACPI, we map in a few
> >selected devices (UART, GIC, SMMU, RAM) to dom0 but leave the rest unmapped.
> >Dom0 then parses ACPI tables and issues hypervisor calls to map individual
> >devices (XENMEM_add_to_physmap with XENMAPSPACE_dev_mmio).
> 
> That is correct.
> 
> >
> >Since XENMEM_add_to_physmap with XENMAPSPACE_dev_mmio is only used
> >for dom0 mappings, I think this relaxation would be safe:
> >+++ b/xen/arch/arm/p2m.c
> >@@ -1185,7 +1185,7 @@ int map_dev_mmio_region(struct domain *d,
> >     if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 
> > 1)) )
> >         return 0;
> >
> >-    res = map_mmio_regions(d, gfn, nr, mfn);
> >+    res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
> >     if ( res < 0 )
> >     {
> 
> This change looks good to me. I will give a try when the new version will be
> sent.

Thanks!

> >
> >Anyway, I'll send the v2 series out and we can discuss from there.
> 
> Can you also please modify the comment on "XENMEMSPACE_dev_mmio" in
> xen/include/public/memory.h regarding the memory attribute used to map?

Yes, I've updated the comment.

Cheers,
Edgar

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