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

[Julien Grall <julien.grall@xxxxxxx>]

(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.
> 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