Xen: ARM: Support for mapping ECAM PCIe Config Space Specified In Static ACPI Table

Xen: ARM: Support for mapping ECAM PCIe Config Space Specified In Static ACPI Table

[Jiandi An]

Hi Guys,

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?

Thanks.

-- 
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

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

[Julien Grall]

On 14/12/16 08:00, Jiandi An wrote:
> Hi Guys,

Hello Jiandi,

> 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?

Cheers,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg01693.html
[2] 
https://lists.xenproject.org/archives/html/xen-devel/2016-12/msg01737.html

-- 
Julien Grall

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

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

[Julien Grall]

Hi,

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

Regards,

-- 
Julien Grall

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

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

[Jaggi, Manish]

Hi,

>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".
>
 Why not add support for parsing ACPI tables in Xen, from linux,  as we parse dt.

-manish


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

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

[Julien Grall]

On 19/12/2016 13:20, Jaggi, Manish 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".
>>
>  Why not add support for parsing ACPI tables in Xen, from linux,  as we parse dt.

Because MMIO can be described in ASL too. I would rather avoid to have a 
different behavior depending whether the MMIO has been described in 
static table or ASL.

Cheers,

-- 
Julien Grall

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

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

[Jiandi An]

On 12/19/16 07:11, Julien Grall wrote:
> 
> 
> On 19/12/2016 13:20, Jaggi, Manish 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".
>>>
>>  Why not add support for parsing ACPI tables in Xen, from linux,  as we parse dt.
> 
> Because MMIO can be described in ASL too. I would rather avoid to have a different behavior depending whether the MMIO has been described in static table or ASL.
> 
> Cheers,
> 

I also think hooking up a call to XENMEM_add_to_physmap_range in ioremap
is not a good approach as ioremap() is commonly called in so many places.
It's not ideal to make a check of am I dom0 running under xen every time
ioremap() is called.  And Julien also pointed out, not every call to ioremap()
needs a stage 2 mapping.

-- 
Jiandi An
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

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

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

[Julien Grall]

Hi Jiandi,

On 20/12/2016 07:31, Jiandi An wrote:
> On 12/19/16 07:11, Julien Grall wrote:
>>
>>
>> On 19/12/2016 13:20, Jaggi, Manish 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".
>>>>
>>>  Why not add support for parsing ACPI tables in Xen, from linux,  as we parse dt.
>>
>> Because MMIO can be described in ASL too. I would rather avoid to have a different behavior depending whether the MMIO has been described in static table or ASL.
>>
>> Cheers,
>>
>
> I also think hooking up a call to XENMEM_add_to_physmap_range in ioremap
> is not a good approach as ioremap() is commonly called in so many places.
> It's not ideal to make a check of am I dom0 running under xen every time
> ioremap() is called.  And Julien also pointed out, not every call to ioremap()
> needs a stage 2 mapping.

I think you misunderstood my previous e-mail. Xen cannot differentiate 
whether an MMIO region is being emulated. So if Xen decides to emulate 
an AMBA device, we would be in the same trouble.

To be clear, in my previous mail I was pointing a drawback of this 
solution. But I believe this is the best way to get the stage-2 mapping 
correct and limiting the size of stage-2 PT for DOM0.

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Tue, 20 Dec 2016, Julien Grall wrote:
> Hi Jiandi,
> 
> On 20/12/2016 07:31, Jiandi An wrote:
> > On 12/19/16 07:11, Julien Grall wrote:
> > > 
> > > 
> > > On 19/12/2016 13:20, Jaggi, Manish 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".
> > > > > 
> > > >  Why not add support for parsing ACPI tables in Xen, from linux,  as we
> > > > parse dt.
> > > 
> > > Because MMIO can be described in ASL too. I would rather avoid to have a
> > > different behavior depending whether the MMIO has been described in static
> > > table or ASL.
> > > 
> > > Cheers,
> > > 
> > 
> > I also think hooking up a call to XENMEM_add_to_physmap_range in ioremap
> > is not a good approach as ioremap() is commonly called in so many places.
> > It's not ideal to make a check of am I dom0 running under xen every time
> > ioremap() is called.  And Julien also pointed out, not every call to
> > ioremap()
> > needs a stage 2 mapping.
> 
> I think you misunderstood my previous e-mail. Xen cannot differentiate whether
> an MMIO region is being emulated. So if Xen decides to emulate an AMBA device,
> we would be in the same trouble.
> 
> To be clear, in my previous mail I was pointing a drawback of this solution.
> But I believe this is the best way to get the stage-2 mapping correct and
> limiting the size of stage-2 PT for DOM0.

Right. And it wouldn't be a general purpose alternative implementation
of ioremap. It would be only for the benefit of acpi.

In fact, looking at the Linux code, I found
include/acpi/acpi_io.h:acpi_os_ioremap. What we want already exists. We
only need to:

- provide a Xen based implementation of acpi_os_ioremap
- make sure that acpi_os_ioremap is called instead of ioremap in all
  instances we care about

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

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

[Stefano Stabellini]

On Mon, 19 Dec 2016, Julien Grall wrote:
> Hi,
> 
> 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.

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

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

[Julien Grall]

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?

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Stefano Stabellini]

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.

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.


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

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

[Julien Grall]

(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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 28.12.16 at 19:22, <julien.grall@arm.com> wrote:
> 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, fwiw.

Jan


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

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

[Stefano Stabellini]

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

Hi Stefano,

On 03/01/17 19:27, Stefano Stabellini wrote:
> On Wed, 28 Dec 2016, Julien Grall wrote:
>> On 20/12/16 22:33, Stefano Stabellini wrote:
>>> On Tue, 20 Dec 2016, Julien Grall wrote:
>>>> 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:
>> 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".

This hypercall is part of the stable ABI. So would it be fine to modify 
the error code?

This hypercall was introduced in Xen 4.7 and used with ACPI. The current 
behavior of Linux is to print a warning if the error code is not 0 and 
then return success. So previous Linux version would print spurious warning.

ACPI support for ARM in the hypervisor is still marked as experimental. 
I would like to see it fully supported in Xen 4.9. Before that, I think 
we should fix the hypercall.

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Mon, 16 Jan 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/01/17 19:27, Stefano Stabellini wrote:
> > On Wed, 28 Dec 2016, Julien Grall wrote:
> > > On 20/12/16 22:33, Stefano Stabellini wrote:
> > > > On Tue, 20 Dec 2016, Julien Grall wrote:
> > > > > 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:
> > > 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".
> 
> This hypercall is part of the stable ABI. So would it be fine to modify the
> error code?
> 
> This hypercall was introduced in Xen 4.7 and used with ACPI. The current
> behavior of Linux is to print a warning if the error code is not 0 and then
> return success. So previous Linux version would print spurious warning.
> 
> ACPI support for ARM in the hypervisor is still marked as experimental. I
> would like to see it fully supported in Xen 4.9. Before that, I think we
> should fix the hypercall.

The possible error codes for XENMEM_add_to_physmap_range are not clearly
listed. I don't think it would be a problem to add a new one.
Additionally, given that ACPI is still marked as experimental, I think
we have a pretty large degree of freedom in this regard.

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