Re: [Xen-devel] Xen: ARM: Support for mapping ECAM PCIe Config Space Specified In Static ACPI Table

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 28 Dec 2016, Julien Grall wrote:
> (CC Andrew and Jan)
> 
> Hi Stefano,
> 
> On 20/12/16 22:33, Stefano Stabellini wrote:
> > On Tue, 20 Dec 2016, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 20/12/2016 00:54, Stefano Stabellini wrote:
> > > > On Mon, 19 Dec 2016, Julien Grall wrote:
> > > > > On 16/12/2016 15:49, Julien Grall wrote:
> > > > > > On 14/12/16 08:00, Jiandi An wrote:
> > > > > > > Xen currently doesn't map ECAM space specified in static ACPI
> > > > > > > table.
> > > > > > > Seeking opinion on how this should be handled properly.
> > > > > > > Each root complex ECAM region takes up 64K 4K pages (256MB).
> > > > > > > For some platforms there might be multiple root complexes.
> > > > > > > Is the plan to map all at once?Julien has mentioned support
> > > > > > > for mapping ECAM may come when support for PCI passthrough is
> > > > > > > added, is that right? What mechanism will it be? Will Xen or
> > > > > > > dom0 be the one that parses the staic ACPI tables and map the ECAM
> > > > > > > space?
> > > > > > 
> > > > > > For performance reason, each ECAM region would need to be mapped at
> > > > > > once, so the stage-2 page table could take advantage of superpage
> > > > > > (it
> > > > > > will mostly be 2MB).
> > > > > > 
> > > > > > Now, I don't think Xen should map the ECAM region in stage-2 before
> > > > > > hand. All the regions may not be described in the MCFG and I would
> > > > > > like
> > > > > > to see a generic solution.
> > > > > > 
> > > > > > Looking at the code (see pci_create_ecam_create in
> > > > > > drivers/pci/ecam.c),
> > > > > > ioremap is used. I believe the problem is the same for the 2 other
> > > > > > threads you sent ( [1] and [2]).
> > > > > > 
> > > > > > So it might be good to look at hooking up a call to
> > > > > > XENMEM_add_to_physmap_range in ioremap.
> > > > > > 
> > > > > > Any opinions?
> > > > > 
> > > > > I thought a bit more about it and I realized we need to be cautious on
> > > > > how
> > > > > to
> > > > > proceed here.
> > > > > 
> > > > > DOM0 will have a mix of real devices and emulated devices (e.g some
> > > > > part
> > > > > of
> > > > > the GIC). For the emulated devices, DOM0 should not call
> > > > > XENMEM_add_to_physmap_range. However, DOM0 is not aware what is
> > > > > emulated
> > > > > or
> > > > > not, so even the current approach (hooking up in platform device)
> > > > > seems
> > > > > fragile. We rely on Xen to say "this region cannot be mapped".
> > > > 
> > > > You are right that Dom0 doesn't and shouldn't be able to tell the
> > > > difference between emulated and real devices. But I don't think that
> > > > should be a problem because Xen knows exactly if an MMIO region belongs
> > > > to an emulated device thanks to the 1:1 mapping. When
> > > > XENMEM_add_to_physmap_range is called, Xen can check whether the region
> > > > belongs to an emulated device or a real device, mapping memory only if
> > > > it belongs to a real device. No need to report errors: from Dom0 point
> > > > of view the operation succeeded either way.
> > > 
> > > By no need to report errors, did you mean Xen failing, or DOM0 failing?
> > 
> > Sorry I haven't been clear. I meant that if Dom0 asks Xen to map a
> > region which belongs to an emulated device, of course Xen won't actually
> > do any mappings, but it is not an error condition. Xen shouldn't return
> > error for mappings that hasn't performed because the region belongs to
> > an emulated device.
> 
> I disagree here. You make the assumption that DOM0 will always issue the
> hypercall with MFN == GFN.
> 
> Today we only check whether the region is allowed in iomem. If it is not
> allowed we will ignore the request and report as "succeeded". But it does not
> mean there will be an emulation behind nor DOM0 decided to map the region with
> MFN != GFN.
> 
> So DOM0 expects the region to be mapped, but actually it may not be done by
> the hypervisor. It will be a pain to debug it because the error may come up
> much later.
> 
> The description of the hypercall is "the region is mapped in Stage-2 using the
> memory attribute Device-nGnRE". Nowhere it is stated the region will not be
> mapped if emulated nor must be called MFN == GFN.
> 
> Now, we have two hypercalls XEN_DOMCTL_memory_mapping and
> XENMEM_add_to_physmap_range having two distinct behavior when mapping an MMIO
> into a guest.
> 
> We should at least return an error, even if DOM0 decides to ignore it.
> 
> I am open to any other suggestion. But I don't think the hypercall should
> silently ignore a request as it is done today.

I agree that assigning clear and unequivocal error codes is a good idea.
The error code for "failure to map" should be different from the error
code for "this belongs to an emulated device".


> > Of course, Xen should report errors for any genuine error conditions.
> > 
> > 
> > > I looked at the code (map_dev_mmio_region), we check whether DOM0 is
> > > allowed
> > > to access the iomem. If a part of the region is not accessible, it will
> > > map
> > > nothing and return as it has succeeded.
> > > 
> > > This behavior is fairly odd because it means that a domain will never know
> > > whether a region has been mapped correctly. It may fail later on because,
> > > for
> > > instance, the domain was trying to map the device with MFN != GFN.
> > > Thankfully
> > > we only map one page at the time (see the caller
> > > xenmem_add_to_physmap_one)
> > > but still a domain will expect to know what's going on.
> > > 
> > > So I think we need to associate a specific errno to tell the domain this
> > > region is not accessible.
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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