Re: [Xen-devel] [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h

[Julien Grall]

Hi Wei,

On 07/07/16 16:28, Wei Liu wrote:

On Thu, Jul 07, 2016 at 09:07:59AM -0600, Jan Beulich wrote:

On 07.07.16 at 16:55,  wrote:

Cc HV maintainers

I'm of course fine with moving this structure somewhere else.

On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote:


diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h



I suspect it would make more sense to move it to public/arch-x86/xen.h.


The question is whether we really mean this to remain x86 specific:
The way it's now there's nothing inherently x86-ish there. But if
that's the plan (which the conditional around it supports) then the
suggested alternative resting place seems appropriate to me.



For one, ARM doesn't distinguish PV vs HVM vs PVH (yet). Calling it HVM
for ARM would be wrong IMHO.


Correct, if you would have to do a comparison with x86 it would be PVH.

However, this structure looks useful only if we lack a way to describe 
those parameters in the firmware. This is not the case for ARM as this 
could be described easily by the device tree with the generic bindings.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h

[Wei Liu]

On Thu, Jul 07, 2016 at 09:07:59AM -0600, Jan Beulich wrote:
> >>> On 07.07.16 at 16:55,  wrote:
> > Cc HV maintainers
> > 
> > I'm of course fine with moving this structure somewhere else.
> > 
> > On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote:
> > 
> >> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > 
> > 
> > I suspect it would make more sense to move it to public/arch-x86/xen.h.
> 
> The question is whether we really mean this to remain x86 specific:
> The way it's now there's nothing inherently x86-ish there. But if
> that's the plan (which the conditional around it supports) then the
> suggested alternative resting place seems appropriate to me.
> 

For one, ARM doesn't distinguish PV vs HVM vs PVH (yet). Calling it HVM
for ARM would be wrong IMHO.

I've CC'ed Stefano and Julien to let them express their opinions.

Wei.

> Jan
> 

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

Re: [Xen-devel] [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h

[Andrew Cooper]

On 07/07/16 16:09, Jan Beulich wrote:
 On 07.07.16 at 17:02,  wrote:
>> On 22/06/16 18:15, Anthony PERARD wrote:
>>> Instead of having several representation of hvm_start_info in C, define
>>> it in public/xen.h so both libxc and hvmloader can use it.
>>>
>>> Signed-off-by: Anthony PERARD 
>>> ---
>>> Change in V5:
>>> - remove packed attribute.
>>>
>>> New in V4.
>>> ---
>>>  tools/libxc/include/xc_dom.h | 31 ---
>>>  xen/include/public/xen.h | 27 +++
>> +1 to the public interface, but I would recommend introducing a new file
>> public/hvm/start_info.h rather than extending the general dumping ground
>> which is xen.h
> But then - see my other reply - perhaps public/arch-x86/hvm/start_info.h?

Fine by me.

~Andrew

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

Re: [Xen-devel] [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h

[Andrew Cooper]

On 22/06/16 18:15, Anthony PERARD wrote:
> Instead of having several representation of hvm_start_info in C, define
> it in public/xen.h so both libxc and hvmloader can use it.
>
> Signed-off-by: Anthony PERARD 
> ---
> Change in V5:
> - remove packed attribute.
>
> New in V4.
> ---
>  tools/libxc/include/xc_dom.h | 31 ---
>  xen/include/public/xen.h | 27 +++

+1 to the public interface, but I would recommend introducing a new file
public/hvm/start_info.h rather than extending the general dumping ground
which is xen.h

~Andrew

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

Re: [Xen-devel] [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h

[Jan Beulich]

>>> On 07.07.16 at 17:02,  wrote:
> On 22/06/16 18:15, Anthony PERARD wrote:
>> Instead of having several representation of hvm_start_info in C, define
>> it in public/xen.h so both libxc and hvmloader can use it.
>>
>> Signed-off-by: Anthony PERARD 
>> ---
>> Change in V5:
>> - remove packed attribute.
>>
>> New in V4.
>> ---
>>  tools/libxc/include/xc_dom.h | 31 ---
>>  xen/include/public/xen.h | 27 +++
> 
> +1 to the public interface, but I would recommend introducing a new file
> public/hvm/start_info.h rather than extending the general dumping ground
> which is xen.h

But then - see my other reply - perhaps public/arch-x86/hvm/start_info.h?

Jan


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

Re: [Xen-devel] [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h

[Jan Beulich]

>>> On 07.07.16 at 16:55,  wrote:
> Cc HV maintainers
> 
> I'm of course fine with moving this structure somewhere else.
> 
> On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote:
> 
>> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> 
> 
> I suspect it would make more sense to move it to public/arch-x86/xen.h.

The question is whether we really mean this to remain x86 specific:
The way it's now there's nothing inherently x86-ish there. But if
that's the plan (which the conditional around it supports) then the
suggested alternative resting place seems appropriate to me.

Jan


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

Re: [Xen-devel] [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h

[Wei Liu]

Cc HV maintainers

I'm of course fine with moving this structure somewhere else.

On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote:

> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h


I suspect it would make more sense to move it to public/arch-x86/xen.h.

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

[Xen-devel] [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h

[Anthony PERARD]

Instead of having several representation of hvm_start_info in C, define
it in public/xen.h so both libxc and hvmloader can use it.

Signed-off-by: Anthony PERARD 
---
Change in V5:
- remove packed attribute.

New in V4.
---
 tools/libxc/include/xc_dom.h | 31 ---
 xen/include/public/xen.h | 27 +++
 2 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index 0629971..de7dca9 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -219,37 +219,6 @@ struct xc_dom_image {
 struct xc_hvm_firmware_module smbios_module;
 };
 
-#if defined(__i386__) || defined(__x86_64__)
-/* C representation of the x86/HVM start info layout.
- *
- * The canonical definition of this layout resides in public/xen.h, this
- * is just a way to represent the layout described there using C types.
- *
- * NB: the packed attribute is not really needed, but it helps us enforce
- * the fact this this is just a representation, and it might indeed
- * be required in the future if there are alignment changes.
- */
-struct hvm_start_info {
-uint32_t magic; /* Contains the magic value 0x336ec578   */
-/* ("xEn3" with the 0x80 bit of the "E" set).*/
-uint32_t version;   /* Version of this structure.*/
-uint32_t flags; /* SIF_xxx flags.*/
-uint32_t nr_modules;/* Number of modules passed to the kernel.   */
-uint64_t modlist_paddr; /* Physical address of an array of   */
-/* hvm_modlist_entry.*/
-uint64_t cmdline_paddr; /* Physical address of the command line. */
-uint64_t rsdp_paddr;/* Physical address of the RSDP ACPI data*/
-/* structure.*/
-} __attribute__((packed));
-
-struct hvm_modlist_entry {
-uint64_t paddr; /* Physical address of the module.   */
-uint64_t size;  /* Size of the module in bytes.  */
-uint64_t cmdline_paddr; /* Physical address of the command line. */
-uint64_t reserved;
-} __attribute__((packed));
-#endif /* x86 */
-
 /* --- pluggable kernel loader - */
 
 struct xc_dom_loader {
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index d9ddee7..defde97 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -859,6 +859,33 @@ typedef struct start_info start_info_t;
  */
 #define XEN_HVM_START_MAGIC_VALUE 0x336ec578
 
+#if defined(__i386__) || defined(__x86_64__)
+/* C representation of the x86/HVM start info layout.
+ *
+ * The canonical definition of this layout is abrove, this is just a way to
+ * represent the layout described there using C types.
+ */
+struct hvm_start_info {
+uint32_t magic; /* Contains the magic value 0x336ec578   */
+/* ("xEn3" with the 0x80 bit of the "E" set).*/
+uint32_t version;   /* Version of this structure.*/
+uint32_t flags; /* SIF_xxx flags.*/
+uint32_t nr_modules;/* Number of modules passed to the kernel.   */
+uint64_t modlist_paddr; /* Physical address of an array of   */
+/* hvm_modlist_entry.*/
+uint64_t cmdline_paddr; /* Physical address of the command line. */
+uint64_t rsdp_paddr;/* Physical address of the RSDP ACPI data*/
+/* structure.*/
+};
+
+struct hvm_modlist_entry {
+uint64_t paddr; /* Physical address of the module.   */
+uint64_t size;  /* Size of the module in bytes.  */
+uint64_t cmdline_paddr; /* Physical address of the command line. */
+uint64_t reserved;
+};
+#endif /* x86 */
+
 /* New console union for dom0 introduced in 0x00030203. */
 #if __XEN_INTERFACE_VERSION__ < 0x00030203
 #define console_mfnconsole.domU.mfn
-- 
Anthony PERARD


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