Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-16 Thread Wei Liu 
On Thu, Jun 16, 2016 at 06:53:37PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/6/16 18:49, Wei Liu wrote:
> > On Tue, Jun 07, 2016 at 08:32:26PM +0800, Shannon Zhao wrote:
> > [...]
> >> > That's its own mechanism I think and UEFI wants all the memory maps
> >> > under its control. And it's same for QEMU(x86 and ARM) and also for Xen
> >> > on x86. You can have a look at the OvmfPkg/AcpiPlatformDxe/Xen.c which
> >> > is used for x86 Xen DomU.
> >> > 
> > I don't think Xen relocates ACPI tables in OVMF. The code scans for
> > signature in code.
> Maybe you can have a look at the function InstallXenTables().
> 

Oh, I thought you were talking about Xen specific code. That's my
misunderstanding.

So that function (InstallXenTables) calls InstallAcpiTable, which
eventually calls AcpiProtocol->InstallAcpiTable.

Does it imply "UEFI wants all memory maps under its control"? Does it
imply UEFI will relocate those tables (or keep a copy to itself)?  The
answer "implementation specific" or "not sure" is not good enough to
address Julien's concern.

Maybe this is just due to miscommunication or using the wrong term. I
will refrain from commenting further now because the issue is quite ARM
specific. Don't want to distract you guys further.

Wei.

> Thanks,
> -- 
> Shannon
> 

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-16 Thread Julien Grall 

Hello Shannon,

On 16/06/16 11:45, Shannon Zhao wrote:

On 2016/6/7 21:06, Julien Grall wrote:

That's its own mechanism I think and UEFI wants all the memory maps
under its control. And it's same for QEMU(x86 and ARM) and also for Xen
on x86. You can have a look at the OvmfPkg/AcpiPlatformDxe/Xen.c which
is used for x86 Xen DomU.


UEFI cannot control the memory map because it is not capable to know
what a region is used for (such as magic pages, grant table...).

In the case of domU for x86, the ACPI table are located in predefine
physical address by hvmloader.

The truth is that hvmloader puts the tables at address
ACPI_PHYSICAL_ADDRESS(0x000EA020), but UEFI will install the tables and
relocate them as well. You can see OvmfPkg/AcpiPlatformDxe/Xen.c in edk2
source code.

So I think it's same with ARM except x86 puts the tables at one fixed
address while ARM dynamically computes the address and passes the
address information to UEFI through dts.

Yeah, of course we can let ARM put the tables at one fixed address and
also it doesn't need the ACPI module to pass the address and let UEFI
find the RSDP table from the fixed address. But what's the difference
between the fixed and dynamicall ones? Because both ways UEFI will
install and relocate the tables.


You seem to think that OVMF is the only UEFI implementation available
and its behavior is set in stone. Can you quote the UEFI spec stating
the ACPI tables will always be relocated?

If not, putting the ACPI module right in the middle of memory is not the
wisest choice because the tables can not be easily relocated like other
modules (DT, Kernel, initramfs).


The ACPI tables are put after the DT. How could you say the ACPI is in
the middle while DT not?


Because the position of the DT in the memory is defined by the Linux 
boot ABI (see section 4.b in Documentation/arm/Booting).





My suggestion to put the ACPI tables at a static address outside the RAM
is to let the choice to the firmware to do whatever it wants without
impacting the performance of a kernel if it ever decides to keep in
place the tables.

However, this static address is from the toolstack point of view. The
firmware should not assume any static address because the guest memory
layout is not part of the ABI for Xen ARM (i.e it can be modified
between two releases). This is to allow us reshuffling the layout to
make space for new features.


Sure? Currently for Xen x86, edk2 uses XEN_ACPI_PHYSICAL_ADDRESS to find
RSDP. If we put the tables at another address, DomU will fail to boot.


I am not sure to follow here. Why do you mention x86 here? My point was 
only for ARM and we are not tight to what x86 does.







I really don't see any reason that would
prevent us to do the same on ARM.

If the UEFI firmware wants to relocate the tables, then fine. But we
should also think about any firmware which may not relocate the tables.

Can you name one firmware except the UEFI?


Sorry I meant any other UEFI implementation (i.e other than OVMF) may
not relocate the tables.

So please tell me the name of other UEFI implementation.


What is the point? All our decision should be based on the specification 
and not ONE specific implementation.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-16 Thread Shannon Zhao 


On 2016/6/16 18:49, Wei Liu wrote:
> On Tue, Jun 07, 2016 at 08:32:26PM +0800, Shannon Zhao wrote:
> [...]
>> > That's its own mechanism I think and UEFI wants all the memory maps
>> > under its control. And it's same for QEMU(x86 and ARM) and also for Xen
>> > on x86. You can have a look at the OvmfPkg/AcpiPlatformDxe/Xen.c which
>> > is used for x86 Xen DomU.
>> > 
> I don't think Xen relocates ACPI tables in OVMF. The code scans for
> signature in code.
Maybe you can have a look at the function InstallXenTables().

Thanks,
-- 
Shannon


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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-16 Thread Wei Liu 
On Tue, Jun 07, 2016 at 08:32:26PM +0800, Shannon Zhao wrote:
[...]
> That's its own mechanism I think and UEFI wants all the memory maps
> under its control. And it's same for QEMU(x86 and ARM) and also for Xen
> on x86. You can have a look at the OvmfPkg/AcpiPlatformDxe/Xen.c which
> is used for x86 Xen DomU.
> 

I don't think Xen relocates ACPI tables in OVMF. The code scans for
signature in code.

I also don't quite get the "control memory map" part. OVMF doesn't
update the memory map, it accepts the memory map provided by hvmloader
as-is. See OvmfPkg/PlatformPei/Xen.c:XenGetE820Map.

Note that I'm talking about this in a pretty out of context way, so I
could be misunderstading what you said.

> > Anyway, you rely on the UEFI firmware to always relocating the tables.
> > What would happen if they decide to remove this behavior?
> Eh, I don't believe this would happen.
> 

I think we can only rely on the spec to be sure.

Wei.

> -- 
> Shannon
> 

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-16 Thread Shannon Zhao 


On 2016/6/16 18:21, Julien Grall wrote:
> 
> 
> On 16/06/16 07:53, Shannon Zhao wrote:
>> Hi Julien,
> 
> Hi Shannon,
> 
>>
>> On 2016/6/7 21:06, Julien Grall wrote:
 That's its own mechanism I think and UEFI wants all the memory maps
 under its control. And it's same for QEMU(x86 and ARM) and also for Xen
 on x86. You can have a look at the OvmfPkg/AcpiPlatformDxe/Xen.c which
 is used for x86 Xen DomU.
>>>
>>> UEFI cannot control the memory map because it is not capable to know
>>> what a region is used for (such as magic pages, grant table...).
>>>
>>> In the case of domU for x86, the ACPI table are located in predefine
>>> physical address by hvmloader.
>> The truth is that hvmloader puts the tables at address
>> ACPI_PHYSICAL_ADDRESS(0x000EA020), but UEFI will install the tables and
>> relocate them as well. You can see OvmfPkg/AcpiPlatformDxe/Xen.c in edk2
>> source code.
>>
>> So I think it's same with ARM except x86 puts the tables at one fixed
>> address while ARM dynamically computes the address and passes the
>> address information to UEFI through dts.
>>
>> Yeah, of course we can let ARM put the tables at one fixed address and
>> also it doesn't need the ACPI module to pass the address and let UEFI
>> find the RSDP table from the fixed address. But what's the difference
>> between the fixed and dynamicall ones? Because both ways UEFI will
>> install and relocate the tables.
> 
> You seem to think that OVMF is the only UEFI implementation available
> and its behavior is set in stone. Can you quote the UEFI spec stating
> the ACPI tables will always be relocated?
> 
> If not, putting the ACPI module right in the middle of memory is not the
> wisest choice because the tables can not be easily relocated like other
> modules (DT, Kernel, initramfs).
> 
The ACPI tables are put after the DT. How could you say the ACPI is in
the middle while DT not?

> My suggestion to put the ACPI tables at a static address outside the RAM
> is to let the choice to the firmware to do whatever it wants without
> impacting the performance of a kernel if it ever decides to keep in
> place the tables.
> 
> However, this static address is from the toolstack point of view. The
> firmware should not assume any static address because the guest memory
> layout is not part of the ABI for Xen ARM (i.e it can be modified
> between two releases). This is to allow us reshuffling the layout to
> make space for new features.
> 
Sure? Currently for Xen x86, edk2 uses XEN_ACPI_PHYSICAL_ADDRESS to find
RSDP. If we put the tables at another address, DomU will fail to boot.

>>
>>> I really don't see any reason that would
>>> prevent us to do the same on ARM.
>>>
>>> If the UEFI firmware wants to relocate the tables, then fine. But we
>>> should also think about any firmware which may not relocate the tables.
>> Can you name one firmware except the UEFI?
> 
> Sorry I meant any other UEFI implementation (i.e other than OVMF) may
> not relocate the tables.
So please tell me the name of other UEFI implementation.

Thanks,
-- 
Shannon


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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-16 Thread Julien Grall 



On 16/06/16 07:53, Shannon Zhao wrote:

Hi Julien,


Hi Shannon,



On 2016/6/7 21:06, Julien Grall wrote:

That's its own mechanism I think and UEFI wants all the memory maps
under its control. And it's same for QEMU(x86 and ARM) and also for Xen
on x86. You can have a look at the OvmfPkg/AcpiPlatformDxe/Xen.c which
is used for x86 Xen DomU.


UEFI cannot control the memory map because it is not capable to know
what a region is used for (such as magic pages, grant table...).

In the case of domU for x86, the ACPI table are located in predefine
physical address by hvmloader.

The truth is that hvmloader puts the tables at address
ACPI_PHYSICAL_ADDRESS(0x000EA020), but UEFI will install the tables and
relocate them as well. You can see OvmfPkg/AcpiPlatformDxe/Xen.c in edk2
source code.

So I think it's same with ARM except x86 puts the tables at one fixed
address while ARM dynamically computes the address and passes the
address information to UEFI through dts.

Yeah, of course we can let ARM put the tables at one fixed address and
also it doesn't need the ACPI module to pass the address and let UEFI
find the RSDP table from the fixed address. But what's the difference
between the fixed and dynamicall ones? Because both ways UEFI will
install and relocate the tables.


You seem to think that OVMF is the only UEFI implementation available 
and its behavior is set in stone. Can you quote the UEFI spec stating 
the ACPI tables will always be relocated?


If not, putting the ACPI module right in the middle of memory is not the 
wisest choice because the tables can not be easily relocated like other 
modules (DT, Kernel, initramfs).


My suggestion to put the ACPI tables at a static address outside the RAM 
is to let the choice to the firmware to do whatever it wants without 
impacting the performance of a kernel if it ever decides to keep in 
place the tables.


However, this static address is from the toolstack point of view. The 
firmware should not assume any static address because the guest memory 
layout is not part of the ABI for Xen ARM (i.e it can be modified 
between two releases). This is to allow us reshuffling the layout to 
make space for new features.





I really don't see any reason that would
prevent us to do the same on ARM.

If the UEFI firmware wants to relocate the tables, then fine. But we
should also think about any firmware which may not relocate the tables.

Can you name one firmware except the UEFI?


Sorry I meant any other UEFI implementation (i.e other than OVMF) may 
not relocate the tables.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-16 Thread Shannon Zhao 
Hi Julien,

On 2016/6/7 21:06, Julien Grall wrote:
>> That's its own mechanism I think and UEFI wants all the memory maps
>> under its control. And it's same for QEMU(x86 and ARM) and also for Xen
>> on x86. You can have a look at the OvmfPkg/AcpiPlatformDxe/Xen.c which
>> is used for x86 Xen DomU.
> 
> UEFI cannot control the memory map because it is not capable to know
> what a region is used for (such as magic pages, grant table...).
> 
> In the case of domU for x86, the ACPI table are located in predefine
> physical address by hvmloader.
The truth is that hvmloader puts the tables at address
ACPI_PHYSICAL_ADDRESS(0x000EA020), but UEFI will install the tables and
relocate them as well. You can see OvmfPkg/AcpiPlatformDxe/Xen.c in edk2
source code.

So I think it's same with ARM except x86 puts the tables at one fixed
address while ARM dynamically computes the address and passes the
address information to UEFI through dts.

Yeah, of course we can let ARM put the tables at one fixed address and
also it doesn't need the ACPI module to pass the address and let UEFI
find the RSDP table from the fixed address. But what's the difference
between the fixed and dynamicall ones? Because both ways UEFI will
install and relocate the tables.

> I really don't see any reason that would
> prevent us to do the same on ARM.
> 
> If the UEFI firmware wants to relocate the tables, then fine. But we
> should also think about any firmware which may not relocate the tables.
Can you name one firmware except the UEFI?
Actually I have the same worries before when I handle the RTC device
problem between UEFI and QEMU. You can see the conversation from below urls.

http://permalink.gmane.org/gmane.comp.bios.edk2.devel/6474
https://lists.gnu.org/archive/html/qemu-arm/2016-01/msg00132.html

Thanks,
-- 
Shannon


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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Julien Grall 



On 07/06/16 13:32, Shannon Zhao wrote:



On 2016/6/7 20:06, Julien Grall wrote:



On 07/06/16 12:59, Shannon Zhao wrote:



On 2016/6/7 19:42, Julien Grall wrote:



On 07/06/16 12:35, Shannon Zhao wrote:



On 2016/6/7 19:27, Julien Grall wrote:



On 07/06/16 12:13, Shannon Zhao wrote:



On 2016/6/7 19:05, Julien Grall wrote:

Hello Shannon,

On 31/05/16 06:02, Shannon Zhao wrote:

From: Shannon Zhao 

Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT,
DSDT
for ARM VM. So only add placeholders for them here.

Signed-off-by: Shannon Zhao 
---
  tools/libxc/include/xc_dom.h | 17 +
  1 file changed, 17 insertions(+)

diff --git a/tools/libxc/include/xc_dom.h
b/tools/libxc/include/xc_dom.h
index 6cb10c4..0fe54dd 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -56,6 +56,20 @@ struct xc_dom_phys {
  xen_pfn_t count;
  };

+struct acpitable {
+void *table;
+size_t size;
+};
+
+struct acpitable_blob {
+struct acpitable rsdp;
+struct acpitable xsdt;
+struct acpitable gtdt;
+struct acpitable madt;
+struct acpitable fadt;
+struct acpitable dsdt;
+};


Is there any particular reason to expose the list of the tables
outside
of the building code?

I would provide a single buffer with all the tables inside.
Similar to
what you did for building the tables in the hypervisor.

When it loads these tables to guest memory space, it needs to update
the
entries (pointing to other tables) of XSDT and also the
xsdt_physical_address in RSDP.


Why can't we load the ACPI tables at an hardcoded place in the memory
(for instance always at the beginning of the RAM)?


I think it's more reasonable to let the codes dynamically compute where
it should put these tables at like what it does for the devicetree
blob.

And to an hardcoded place, can you make sure that kind of place is
always available and not used by others?


Yes, the toolstack is in charge of the memory layout. So it can ensure
that no-one else is using this region.

My concern is, based on you patch #13, the ACPI tables are allocated
just after all the other modules. However, they cannot be relocated by
the kernel because they contain physical address. So they have to stay
in place for all the life of the domain.


No, this doesn't like what you say. UEFI will install and relocate the
ACPI tables again for guest kernel. That means the ACPI tables guest
gets are not from the original place where toolstack puts them at.


Why does UEFI need to relocate the ACPI tables?


That's its own mechanism I think and UEFI wants all the memory maps
under its control. And it's same for QEMU(x86 and ARM) and also for Xen
on x86. You can have a look at the OvmfPkg/AcpiPlatformDxe/Xen.c which
is used for x86 Xen DomU.


UEFI cannot control the memory map because it is not capable to know 
what a region is used for (such as magic pages, grant table...).


In the case of domU for x86, the ACPI table are located in predefine 
physical address by hvmloader. I really don't see any reason that would 
prevent us to do the same on ARM.


If the UEFI firmware wants to relocate the tables, then fine. But we 
should also think about any firmware which may not relocate the tables.





Anyway, you rely on the UEFI firmware to always relocating the tables.
What would happen if they decide to remove this behavior?

Eh, I don't believe this would happen.


You seem very optimistic. Xen does not rely on a specific UEFI 
implementation nor how a guest will behave. We have to predict what 
could happen and find the best way to do it.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Shannon Zhao 


On 2016/6/7 20:06, Julien Grall wrote:
> 
> 
> On 07/06/16 12:59, Shannon Zhao wrote:
>>
>>
>> On 2016/6/7 19:42, Julien Grall wrote:
>>>
>>>
>>> On 07/06/16 12:35, Shannon Zhao wrote:


 On 2016/6/7 19:27, Julien Grall wrote:
>
>
> On 07/06/16 12:13, Shannon Zhao wrote:
>>
>>
>> On 2016/6/7 19:05, Julien Grall wrote:
>>> Hello Shannon,
>>>
>>> On 31/05/16 06:02, Shannon Zhao wrote:
 From: Shannon Zhao 

 Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT,
 DSDT
 for ARM VM. So only add placeholders for them here.

 Signed-off-by: Shannon Zhao 
 ---
  tools/libxc/include/xc_dom.h | 17 +
  1 file changed, 17 insertions(+)

 diff --git a/tools/libxc/include/xc_dom.h
 b/tools/libxc/include/xc_dom.h
 index 6cb10c4..0fe54dd 100644
 --- a/tools/libxc/include/xc_dom.h
 +++ b/tools/libxc/include/xc_dom.h
 @@ -56,6 +56,20 @@ struct xc_dom_phys {
  xen_pfn_t count;
  };

 +struct acpitable {
 +void *table;
 +size_t size;
 +};
 +
 +struct acpitable_blob {
 +struct acpitable rsdp;
 +struct acpitable xsdt;
 +struct acpitable gtdt;
 +struct acpitable madt;
 +struct acpitable fadt;
 +struct acpitable dsdt;
 +};
>>>
>>> Is there any particular reason to expose the list of the tables
>>> outside
>>> of the building code?
>>>
>>> I would provide a single buffer with all the tables inside.
>>> Similar to
>>> what you did for building the tables in the hypervisor.
>> When it loads these tables to guest memory space, it needs to update
>> the
>> entries (pointing to other tables) of XSDT and also the
>> xsdt_physical_address in RSDP.
>
> Why can't we load the ACPI tables at an hardcoded place in the memory
> (for instance always at the beginning of the RAM)?
>
 I think it's more reasonable to let the codes dynamically compute where
 it should put these tables at like what it does for the devicetree
 blob.

 And to an hardcoded place, can you make sure that kind of place is
 always available and not used by others?
>>>
>>> Yes, the toolstack is in charge of the memory layout. So it can ensure
>>> that no-one else is using this region.
>>>
>>> My concern is, based on you patch #13, the ACPI tables are allocated
>>> just after all the other modules. However, they cannot be relocated by
>>> the kernel because they contain physical address. So they have to stay
>>> in place for all the life of the domain.
>>>
>> No, this doesn't like what you say. UEFI will install and relocate the
>> ACPI tables again for guest kernel. That means the ACPI tables guest
>> gets are not from the original place where toolstack puts them at.
> 
> Why does UEFI need to relocate the ACPI tables?
> 
That's its own mechanism I think and UEFI wants all the memory maps
under its control. And it's same for QEMU(x86 and ARM) and also for Xen
on x86. You can have a look at the OvmfPkg/AcpiPlatformDxe/Xen.c which
is used for x86 Xen DomU.

> Anyway, you rely on the UEFI firmware to always relocating the tables.
> What would happen if they decide to remove this behavior?
Eh, I don't believe this would happen.

-- 
Shannon


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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Shannon Zhao 


On 2016/6/7 20:02, Julien Grall wrote:
> Hello Shannon,
> 
> On 07/06/16 12:42, Julien Grall wrote:
>> Is there any particular reason to expose the list of the tables
>> outside
>> of the building code?
>>
>> I would provide a single buffer with all the tables inside.
>> Similar to
>> what you did for building the tables in the hypervisor.
> When it loads these tables to guest memory space, it needs to update
> the
> entries (pointing to other tables) of XSDT and also the
> xsdt_physical_address in RSDP.

 Why can't we load the ACPI tables at an hardcoded place in the memory
 (for instance always at the beginning of the RAM)?

>>> I think it's more reasonable to let the codes dynamically compute where
>>> it should put these tables at like what it does for the devicetree blob.
>>>
>>> And to an hardcoded place, can you make sure that kind of place is
>>> always available and not used by others?
>>
>> Yes, the toolstack is in charge of the memory layout. So it can ensure
>> that no-one else is using this region.
>>
>> My concern is, based on you patch #13, the ACPI tables are allocated
>> just after all the other modules. However, they cannot be relocated by
>> the kernel because they contain physical address. So they have to stay
>> in place for all the life of the domain.
>>
>> We should put them in a place where it will not impact the memory
>> allocation of the guest. The start of the RAM is a good place for that.
> 
> I though a bit more on this suggestion. If the ACPI tables are put at
> the beginning of the RAM, a guest may not be able to use super page.
> 
> I would suggest to move the ACPI table out of the real RAM to avoid any
> potential issue with the kernel memory allocation.
> 
> For instance we could define a IPA range to be use for ACPI (e.g
> 0x2000 - 0x2020) and expose to the guest using the ACPI_NVS type
> in the UEFI memory map.
> 
> Any opinion on this?
No, this will not work. UEFI will control the memory map by itself.

-- 
Shannon


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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Julien Grall 



On 07/06/16 12:59, Shannon Zhao wrote:



On 2016/6/7 19:42, Julien Grall wrote:



On 07/06/16 12:35, Shannon Zhao wrote:



On 2016/6/7 19:27, Julien Grall wrote:



On 07/06/16 12:13, Shannon Zhao wrote:



On 2016/6/7 19:05, Julien Grall wrote:

Hello Shannon,

On 31/05/16 06:02, Shannon Zhao wrote:

From: Shannon Zhao 

Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT, DSDT
for ARM VM. So only add placeholders for them here.

Signed-off-by: Shannon Zhao 
---
 tools/libxc/include/xc_dom.h | 17 +
 1 file changed, 17 insertions(+)

diff --git a/tools/libxc/include/xc_dom.h
b/tools/libxc/include/xc_dom.h
index 6cb10c4..0fe54dd 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -56,6 +56,20 @@ struct xc_dom_phys {
 xen_pfn_t count;
 };

+struct acpitable {
+void *table;
+size_t size;
+};
+
+struct acpitable_blob {
+struct acpitable rsdp;
+struct acpitable xsdt;
+struct acpitable gtdt;
+struct acpitable madt;
+struct acpitable fadt;
+struct acpitable dsdt;
+};


Is there any particular reason to expose the list of the tables
outside
of the building code?

I would provide a single buffer with all the tables inside. Similar to
what you did for building the tables in the hypervisor.

When it loads these tables to guest memory space, it needs to update
the
entries (pointing to other tables) of XSDT and also the
xsdt_physical_address in RSDP.


Why can't we load the ACPI tables at an hardcoded place in the memory
(for instance always at the beginning of the RAM)?


I think it's more reasonable to let the codes dynamically compute where
it should put these tables at like what it does for the devicetree blob.

And to an hardcoded place, can you make sure that kind of place is
always available and not used by others?


Yes, the toolstack is in charge of the memory layout. So it can ensure
that no-one else is using this region.

My concern is, based on you patch #13, the ACPI tables are allocated
just after all the other modules. However, they cannot be relocated by
the kernel because they contain physical address. So they have to stay
in place for all the life of the domain.


No, this doesn't like what you say. UEFI will install and relocate the
ACPI tables again for guest kernel. That means the ACPI tables guest
gets are not from the original place where toolstack puts them at.


Why does UEFI need to relocate the ACPI tables?

Anyway, you rely on the UEFI firmware to always relocating the tables. 
What would happen if they decide to remove this behavior?


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Shannon Zhao 


On 2016/6/7 19:42, Julien Grall wrote:
> 
> 
> On 07/06/16 12:35, Shannon Zhao wrote:
>>
>>
>> On 2016/6/7 19:27, Julien Grall wrote:
>>>
>>>
>>> On 07/06/16 12:13, Shannon Zhao wrote:


 On 2016/6/7 19:05, Julien Grall wrote:
> Hello Shannon,
>
> On 31/05/16 06:02, Shannon Zhao wrote:
>> From: Shannon Zhao 
>>
>> Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT, DSDT
>> for ARM VM. So only add placeholders for them here.
>>
>> Signed-off-by: Shannon Zhao 
>> ---
>> tools/libxc/include/xc_dom.h | 17 +
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/tools/libxc/include/xc_dom.h
>> b/tools/libxc/include/xc_dom.h
>> index 6cb10c4..0fe54dd 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -56,6 +56,20 @@ struct xc_dom_phys {
>> xen_pfn_t count;
>> };
>>
>> +struct acpitable {
>> +void *table;
>> +size_t size;
>> +};
>> +
>> +struct acpitable_blob {
>> +struct acpitable rsdp;
>> +struct acpitable xsdt;
>> +struct acpitable gtdt;
>> +struct acpitable madt;
>> +struct acpitable fadt;
>> +struct acpitable dsdt;
>> +};
>
> Is there any particular reason to expose the list of the tables
> outside
> of the building code?
>
> I would provide a single buffer with all the tables inside. Similar to
> what you did for building the tables in the hypervisor.
 When it loads these tables to guest memory space, it needs to update
 the
 entries (pointing to other tables) of XSDT and also the
 xsdt_physical_address in RSDP.
>>>
>>> Why can't we load the ACPI tables at an hardcoded place in the memory
>>> (for instance always at the beginning of the RAM)?
>>>
>> I think it's more reasonable to let the codes dynamically compute where
>> it should put these tables at like what it does for the devicetree blob.
>>
>> And to an hardcoded place, can you make sure that kind of place is
>> always available and not used by others?
> 
> Yes, the toolstack is in charge of the memory layout. So it can ensure
> that no-one else is using this region.
> 
> My concern is, based on you patch #13, the ACPI tables are allocated
> just after all the other modules. However, they cannot be relocated by
> the kernel because they contain physical address. So they have to stay
> in place for all the life of the domain.
> 
No, this doesn't like what you say. UEFI will install and relocate the
ACPI tables again for guest kernel. That means the ACPI tables guest
gets are not from the original place where toolstack puts them at.

-- 
Shannon


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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Julien Grall 

Hello Shannon,

On 07/06/16 12:42, Julien Grall wrote:

Is there any particular reason to expose the list of the tables
outside
of the building code?

I would provide a single buffer with all the tables inside. Similar to
what you did for building the tables in the hypervisor.

When it loads these tables to guest memory space, it needs to update
the
entries (pointing to other tables) of XSDT and also the
xsdt_physical_address in RSDP.


Why can't we load the ACPI tables at an hardcoded place in the memory
(for instance always at the beginning of the RAM)?


I think it's more reasonable to let the codes dynamically compute where
it should put these tables at like what it does for the devicetree blob.

And to an hardcoded place, can you make sure that kind of place is
always available and not used by others?


Yes, the toolstack is in charge of the memory layout. So it can ensure
that no-one else is using this region.

My concern is, based on you patch #13, the ACPI tables are allocated
just after all the other modules. However, they cannot be relocated by
the kernel because they contain physical address. So they have to stay
in place for all the life of the domain.

We should put them in a place where it will not impact the memory
allocation of the guest. The start of the RAM is a good place for that.


I though a bit more on this suggestion. If the ACPI tables are put at 
the beginning of the RAM, a guest may not be able to use super page.


I would suggest to move the ACPI table out of the real RAM to avoid any 
potential issue with the kernel memory allocation.


For instance we could define a IPA range to be use for ACPI (e.g 
0x2000 - 0x2020) and expose to the guest using the ACPI_NVS type 
in the UEFI memory map.


Any opinion on this?

Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Julien Grall 



On 07/06/16 12:35, Shannon Zhao wrote:



On 2016/6/7 19:27, Julien Grall wrote:



On 07/06/16 12:13, Shannon Zhao wrote:



On 2016/6/7 19:05, Julien Grall wrote:

Hello Shannon,

On 31/05/16 06:02, Shannon Zhao wrote:

From: Shannon Zhao 

Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT, DSDT
for ARM VM. So only add placeholders for them here.

Signed-off-by: Shannon Zhao 
---
tools/libxc/include/xc_dom.h | 17 +
1 file changed, 17 insertions(+)

diff --git a/tools/libxc/include/xc_dom.h
b/tools/libxc/include/xc_dom.h
index 6cb10c4..0fe54dd 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -56,6 +56,20 @@ struct xc_dom_phys {
xen_pfn_t count;
};

+struct acpitable {
+void *table;
+size_t size;
+};
+
+struct acpitable_blob {
+struct acpitable rsdp;
+struct acpitable xsdt;
+struct acpitable gtdt;
+struct acpitable madt;
+struct acpitable fadt;
+struct acpitable dsdt;
+};


Is there any particular reason to expose the list of the tables outside
of the building code?

I would provide a single buffer with all the tables inside. Similar to
what you did for building the tables in the hypervisor.

When it loads these tables to guest memory space, it needs to update the
entries (pointing to other tables) of XSDT and also the
xsdt_physical_address in RSDP.


Why can't we load the ACPI tables at an hardcoded place in the memory
(for instance always at the beginning of the RAM)?


I think it's more reasonable to let the codes dynamically compute where
it should put these tables at like what it does for the devicetree blob.

And to an hardcoded place, can you make sure that kind of place is
always available and not used by others?


Yes, the toolstack is in charge of the memory layout. So it can ensure 
that no-one else is using this region.


My concern is, based on you patch #13, the ACPI tables are allocated 
just after all the other modules. However, they cannot be relocated by 
the kernel because they contain physical address. So they have to stay 
in place for all the life of the domain.


We should put them in a place where it will not impact the memory 
allocation of the guest. The start of the RAM is a good place for that.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Shannon Zhao 


On 2016/6/7 19:27, Julien Grall wrote:
> 
> 
> On 07/06/16 12:13, Shannon Zhao wrote:
>>
>>
>> On 2016/6/7 19:05, Julien Grall wrote:
>>> Hello Shannon,
>>>
>>> On 31/05/16 06:02, Shannon Zhao wrote:
 From: Shannon Zhao 

 Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT, DSDT
 for ARM VM. So only add placeholders for them here.

 Signed-off-by: Shannon Zhao 
 ---
tools/libxc/include/xc_dom.h | 17 +
1 file changed, 17 insertions(+)

 diff --git a/tools/libxc/include/xc_dom.h
 b/tools/libxc/include/xc_dom.h
 index 6cb10c4..0fe54dd 100644
 --- a/tools/libxc/include/xc_dom.h
 +++ b/tools/libxc/include/xc_dom.h
 @@ -56,6 +56,20 @@ struct xc_dom_phys {
xen_pfn_t count;
};

 +struct acpitable {
 +void *table;
 +size_t size;
 +};
 +
 +struct acpitable_blob {
 +struct acpitable rsdp;
 +struct acpitable xsdt;
 +struct acpitable gtdt;
 +struct acpitable madt;
 +struct acpitable fadt;
 +struct acpitable dsdt;
 +};
>>>
>>> Is there any particular reason to expose the list of the tables outside
>>> of the building code?
>>>
>>> I would provide a single buffer with all the tables inside. Similar to
>>> what you did for building the tables in the hypervisor.
>> When it loads these tables to guest memory space, it needs to update the
>> entries (pointing to other tables) of XSDT and also the
>> xsdt_physical_address in RSDP.
> 
> Why can't we load the ACPI tables at an hardcoded place in the memory
> (for instance always at the beginning of the RAM)?
> 
I think it's more reasonable to let the codes dynamically compute where
it should put these tables at like what it does for the devicetree blob.

And to an hardcoded place, can you make sure that kind of place is
always available and not used by others?

-- 
Shannon


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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Julien Grall 



On 07/06/16 12:13, Shannon Zhao wrote:



On 2016/6/7 19:05, Julien Grall wrote:

Hello Shannon,

On 31/05/16 06:02, Shannon Zhao wrote:

From: Shannon Zhao 

Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT, DSDT
for ARM VM. So only add placeholders for them here.

Signed-off-by: Shannon Zhao 
---
   tools/libxc/include/xc_dom.h | 17 +
   1 file changed, 17 insertions(+)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 6cb10c4..0fe54dd 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -56,6 +56,20 @@ struct xc_dom_phys {
   xen_pfn_t count;
   };

+struct acpitable {
+void *table;
+size_t size;
+};
+
+struct acpitable_blob {
+struct acpitable rsdp;
+struct acpitable xsdt;
+struct acpitable gtdt;
+struct acpitable madt;
+struct acpitable fadt;
+struct acpitable dsdt;
+};


Is there any particular reason to expose the list of the tables outside
of the building code?

I would provide a single buffer with all the tables inside. Similar to
what you did for building the tables in the hypervisor.

When it loads these tables to guest memory space, it needs to update the
entries (pointing to other tables) of XSDT and also the
xsdt_physical_address in RSDP.


Why can't we load the ACPI tables at an hardcoded place in the memory 
(for instance always at the beginning of the RAM)?


This would make the code a lot simpler and would avoid a duplication of 
the same 5 lines for each tables in patch 14:


+xsdt = (struct acpi_xsdt_descriptor *)acpitablemap;
+memcpy(acpitablemap, dom->acpitable_blob->xsdt.table,
+   dom->acpitable_blob->xsdt.size);

[...]

+start += dom->acpitable_blob->xsdt.size;
+acpitablemap += dom->acpitable_blob->xsdt.size;

Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Shannon Zhao 


On 2016/6/7 19:05, Julien Grall wrote:
> Hello Shannon,
> 
> On 31/05/16 06:02, Shannon Zhao wrote:
>> From: Shannon Zhao 
>>
>> Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT, DSDT
>> for ARM VM. So only add placeholders for them here.
>>
>> Signed-off-by: Shannon Zhao 
>> ---
>>   tools/libxc/include/xc_dom.h | 17 +
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index 6cb10c4..0fe54dd 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -56,6 +56,20 @@ struct xc_dom_phys {
>>   xen_pfn_t count;
>>   };
>>
>> +struct acpitable {
>> +void *table;
>> +size_t size;
>> +};
>> +
>> +struct acpitable_blob {
>> +struct acpitable rsdp;
>> +struct acpitable xsdt;
>> +struct acpitable gtdt;
>> +struct acpitable madt;
>> +struct acpitable fadt;
>> +struct acpitable dsdt;
>> +};
> 
> Is there any particular reason to expose the list of the tables outside
> of the building code?
> 
> I would provide a single buffer with all the tables inside. Similar to
> what you did for building the tables in the hypervisor.
When it loads these tables to guest memory space, it needs to update the
entries (pointing to other tables) of XSDT and also the
xsdt_physical_address in RSDP.

So it needs to know the length of each table and copy them separately.

Please see patch 14.

Thanks,
-- 
Shannon


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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-07 Thread Julien Grall 

Hello Shannon,

On 31/05/16 06:02, Shannon Zhao wrote:

From: Shannon Zhao 

Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT, DSDT
for ARM VM. So only add placeholders for them here.

Signed-off-by: Shannon Zhao 
---
  tools/libxc/include/xc_dom.h | 17 +
  1 file changed, 17 insertions(+)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 6cb10c4..0fe54dd 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -56,6 +56,20 @@ struct xc_dom_phys {
  xen_pfn_t count;
  };

+struct acpitable {
+void *table;
+size_t size;
+};
+
+struct acpitable_blob {
+struct acpitable rsdp;
+struct acpitable xsdt;
+struct acpitable gtdt;
+struct acpitable madt;
+struct acpitable fadt;
+struct acpitable dsdt;
+};


Is there any particular reason to expose the list of the tables outside 
of the building code?


I would provide a single buffer with all the tables inside. Similar to 
what you did for building the tables in the hypervisor.



+
  struct xc_dom_image {
  /* files */
  void *kernel_blob;
@@ -64,6 +78,8 @@ struct xc_dom_image {
  size_t ramdisk_size;
  void *devicetree_blob;
  size_t devicetree_size;
+struct acpitable_blob *acpitable_blob;
+size_t acpitable_size;

  size_t max_kernel_size;
  size_t max_ramdisk_size;
@@ -92,6 +108,7 @@ struct xc_dom_image {
  struct xc_dom_seg p2m_seg;
  struct xc_dom_seg pgtables_seg;
  struct xc_dom_seg devicetree_seg;
+struct xc_dom_seg acpi_seg;
  struct xc_dom_seg start_info_seg; /* HVMlite only */
  xen_pfn_t start_info_pfn;
  xen_pfn_t console_pfn;



Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-06 Thread Shannon Zhao 
On 2016年06月06日 20:16, Wei Liu wrote:
> On Mon, Jun 06, 2016 at 11:00:37AM +0100, Stefano Stabellini wrote:
>> > On Tue, 31 May 2016, Shannon Zhao wrote:
>>> > > From: Shannon Zhao 
>>> > > 
>>> > > Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT, DSDT
>>> > > for ARM VM. So only add placeholders for them here.
>>> > > 
>>> > > Signed-off-by: Shannon Zhao 
>> > 
>> > If we are going to make this ARM only, then maybe we should consider
>> > moving these structs to an ARM specific header?
>> > 
> Agreed. Or at least have some ifdefs around the fields.
> 
Ok, will change. Thanks.

-- 
Shannon

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

Re: [Xen-devel] [PATCH RESEND 03/14] libxc: Add placeholders for ACPI tables blob and size

2016-06-06 Thread Wei Liu 
On Mon, Jun 06, 2016 at 11:00:37AM +0100, Stefano Stabellini wrote:
> On Tue, 31 May 2016, Shannon Zhao wrote:
> > From: Shannon Zhao 
> > 
> > Currently it only needs ACPI table RSDP, XSDT, GTDT, MADT, FADT, DSDT
> > for ARM VM. So only add placeholders for them here.
> > 
> > Signed-off-by: Shannon Zhao 
> 
> If we are going to make this ARM only, then maybe we should consider
> moving these structs to an ARM specific header?
> 

Agreed. Or at least have some ifdefs around the fields.

> 
> >  tools/libxc/include/xc_dom.h | 17 +
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> > index 6cb10c4..0fe54dd 100644
> > --- a/tools/libxc/include/xc_dom.h
> > +++ b/tools/libxc/include/xc_dom.h
> > @@ -56,6 +56,20 @@ struct xc_dom_phys {
> >  xen_pfn_t count;
> >  };
> >  
> > +struct acpitable {
> > +void *table;
> > +size_t size;
> > +};
> > +
> > +struct acpitable_blob {
> > +struct acpitable rsdp;
> > +struct acpitable xsdt;
> > +struct acpitable gtdt;
> > +struct acpitable madt;
> > +struct acpitable fadt;
> > +struct acpitable dsdt;
> > +};
> > +
> >  struct xc_dom_image {
> >  /* files */
> >  void *kernel_blob;
> > @@ -64,6 +78,8 @@ struct xc_dom_image {
> >  size_t ramdisk_size;
> >  void *devicetree_blob;
> >  size_t devicetree_size;
> > +struct acpitable_blob *acpitable_blob;
> > +size_t acpitable_size;
> >  
> >  size_t max_kernel_size;
> >  size_t max_ramdisk_size;
> > @@ -92,6 +108,7 @@ struct xc_dom_image {
> >  struct xc_dom_seg p2m_seg;
> >  struct xc_dom_seg pgtables_seg;
> >  struct xc_dom_seg devicetree_seg;
> > +struct xc_dom_seg acpi_seg;
> >  struct xc_dom_seg start_info_seg; /* HVMlite only */
> >  xen_pfn_t start_info_pfn;
> >  xen_pfn_t console_pfn;
> > -- 
> > 2.0.4
> > 
> > 

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