SMBios table support in XEN

Hi Alex:
	This patch used to get XEN version info and pass to GFW
throuhg
Hob. I tested it with other pathes which for GFW, OSI
interface works
fine with linux kernel 2.6.22.3.

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

On Thu, 2007-08-23 at 19:00 +0800, Zhang, Xing Z wrote:
> Hi Alex:
> 	This patch used to get XEN version info and pass to
GFW throuhg
> Hob. I tested it with other pathes which for GFW, OSI
interface works
> fine with linux kernel 2.6.22.3.

Hi,

   Two comments.  First, check your indenting, it looks like
you've got
tab indenting additions to space indented files in a few
places.
Second, defining and using XENVER_UUID_LEN looks like a
separate patch
that should be sent to xen-devel.  I don't see why there
would be any
problem getting it in, but we can just as easily use 16
until we
standardize on the macro.  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source
& Linux Org.


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

Hi Tristan:
	Pls defer to checke in this patch. Seems some info of
SMBios still wrong and windows depends on these info(I met
some windows guest crash while other windows guest are
fine). I will go on investigate into it. sorry for
in-convenience. 

>-----Original Message-----
>From: xen-ia64-devel-bounceslists.xensource.com 
>[mailtoen-ia
64-devel-bounceslists.xensource.com] On Behalf 
>Of Zhang, Xing Z
>Sent: 2007年8月23日 19:01
>To: Alex Williamson
>Cc: xen-ia64-devel
>Subject: [Xen-ia64-devel][PATCH] SMBios table support in
XEN
>
>Hi Alex:
>	This patch used to get XEN version info and pass to GFW
throuhg
>Hob. I tested it with other pathes which for GFW, OSI
interface works
>fine with linux kernel 2.6.22.3.
>

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

This one is for your comments. I also clean up some
redundant spaces. 
For XENVER_UUID_LEN, I temp define it in xc_ia64_hvm_build.c
and it 
would be removed when IA32 standardize on the marco.

>-----Original Message-----
>From: Alex Williamson [mailto:alex.williamsonhp.com] 
>Sent: 2007年8月23日 21:56
>To: Zhang, Xing Z
>Cc: xen-ia64-devel
>Subject: Re: [Xen-ia64-devel][PATCH] SMBios table
support in XEN
>
>On Thu, 2007-08-23 at 19:00 +0800, Zhang, Xing Z wrote:
>> Hi Alex:
>> 	This patch used to get XEN version info and pass
to GFW throuhg
>> Hob. I tested it with other pathes which for GFW,
OSI interface works
>> fine with linux kernel 2.6.22.3.
>
>Hi,
>
>   Two comments.  First, check your indenting, it looks
like you've got
>tab indenting additions to space indented files in a few
places.
>Second, defining and using XENVER_UUID_LEN looks like a
separate patch
>that should be sent to xen-devel.  I don't see why there
would be any
>problem getting it in, but we can just as easily use 16
until we
>standardize on the macro.  Thanks,
>
>	Alex
>
>-- 
>Alex Williamson                             HP Open
Source & Linux Org.
>

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

Hi Tristan:
	Pls use this one. the old one is not careful handle
unaligned issue due to some code is ported from X86 side.
The new one fixes these issues and works fine.

>-----Original Message-----
>From: Zhang, Xing Z 
>Sent: 2007年8月24日 11:38
>To: Zhang, Xing Z; Alex Williamson
>Cc: xen-ia64-devel
>Subject: RE: [Xen-ia64-devel][PATCH] SMBios table
support in XEN
>
>Hi Tristan:
>	Pls defer to checke in this patch. Seems some info of 
>SMBios still wrong and windows depends on these info(I
met 
>some windows guest crash while other windows guest are
fine). 
>I will go on investigate into it. sorry for
in-convenience. 
>
>>-----Original Message-----
>>From: xen-ia64-devel-bounceslists.xensource.com 
>>[mailtoen-ia
64-devel-bounceslists.xensource.com] On Behalf 
>>Of Zhang, Xing Z
>>Sent: 2007年8月23日 19:01
>>To: Alex Williamson
>>Cc: xen-ia64-devel
>>Subject: [Xen-ia64-devel][PATCH] SMBios table
support in XEN
>>
>>Hi Alex:
>>	This patch used to get XEN version info and pass to
GFW throuhg
>>Hob. I tested it with other pathes which for GFW,
OSI interface works
>>fine with linux kernel 2.6.22.3.
>>

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

On Fri, 2007-08-24 at 13:11 +0800, Zhang, Xing Z wrote:
> This one is for your comments. I also clean up some
redundant spaces. 
> For XENVER_UUID_LEN, I temp define it in
xc_ia64_hvm_build.c and it 
> would be removed when IA32 standardize on the marco.

   It doesn't look like that change is necessarily going to
make it in,
although Keir did make some changes to use
xen_domain_handle_t.  This
patch probably needs another update to follow those changes.
 Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source
& Linux Org.


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

Do you mean we only need temp hard code uuid length to 16
and waiting for keir make xen_domain_handle_t availabe in
libxc? 

>-----Original Message-----
>From: Alex Williamson [mailto:alex.williamsonhp.com] 
>Sent: 2007年8月25日 5:43
>To: Zhang, Xing Z
>Cc: xen-ia64-devel
>Subject: RE: [Xen-ia64-devel][PATCH] SMBios table
support in XEN
>
>On Fri, 2007-08-24 at 13:11 +0800, Zhang, Xing Z wrote:
>> This one is for your comments. I also clean up some

>redundant spaces. 
>> For XENVER_UUID_LEN, I temp define it in
xc_ia64_hvm_build.c and it 
>> would be removed when IA32 standardize on the
marco.
>
>   It doesn't look like that change is necessarily going
to make it in,
>although Keir did make some changes to use
xen_domain_handle_t.  This
>patch probably needs another update to follow those
changes.  Thanks,
>
>	Alex
>
>-- 
>Alex Williamson                             HP Open
Source & Linux Org.
>

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

On Fri, Aug 24, 2007 at 04:02:30PM +0800, Zhang, Xing Z
wrote:
> Hi Tristan:
> 	Pls use this one. the old one is not careful handle
unaligned issue due to some code is ported from X86 side.
The new one fixes these issues and works fine.
Applied.

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

Hi,

   I'll point out the specific place I was thinking below,
along with a
couple other issues.  When I test this with the latest GFW,
I'm also
getting a couple of these on domain startup:

xencomm_privcmd_xen_version: unknown version op 17

And I'm not seeing an SMBIOS table with dmidecode in the
guest.  Is
there still something missing?  Thanks,

	Alex


On Fri, 2007-08-24 at 13:11 +0800, Zhang, Xing Z wrote:
> +/* Founctions for building SMBios table */
> +static void hypercall_xen_version(int xc_handle,
domid_t dom, int
> type, void *val, int len)
> +{

   I think this needs to be more strict in it's
"type" validation.
Currently XENVER_guest_handle returns one thing, anything at
all passed
to it returns a version.  The version stuff seems a little
redundant
with xc_version().  Could we use that instead?

> +    if ( type != XENVER_guest_handle )
> +    {
> +        DECLARE_HYPERCALL;
> +
> +        hypercall.op = __HYPERVISOR_xen_version;
> +        hypercall.arg[0] = __HYPERVISOR_xen_version;
> +
> +        lock_pages(val, len);
> +        do_xen_hypercall(xc_handle, &hypercall);
> +        unlock_pages(val, len);
> +
> +        memcpy(val, &hypercall.arg[1], len);
> +    }
> +    else
> +    {
> +        DECLARE_DOMCTL;
> +
> +        domctl.cmd = XEN_DOMCTL_getdomaininfo;
> +        domctl.domain = dom;
> +
> +        if (xc_domctl(xc_handle, &domctl) < 0)
> +        {
> +            PERROR("Get XEN UUID failed, SMBios
may be built wrong!
> n");
> +            memset(val, 0x0, len);
> +            return;
> +        }
> +
> +        memcpy(val, domctl.u.getdomaininfo.handle,
len);
> +    }
>  }
...
>  -1068,15 +1142,31  setup_guest(int xc_handle,
uint32_t dom,
>  
>      vcpus = domctl.u.getdomaininfo.max_vcpu_id + 1;
>  
> -    // Hand-off state passed to guest firmware 
> -    if (xc_ia64_build_hob(xc_handle, dom, dom_memsize,
vcpus,
> nvram_start) < 0) {
> +    // Get Xen version info for building SMBios table
> +    {
> +        uint8_t uuid[XENVER_UUID_LEN]; // This will
break if
> xen_domain_handle_t is not uint8_t[16]

   This is where I thought maybe we should use a
xen_domain_handle_t
like Keir did in uptream:

    xen_domain_handle_t uuid;

    ...

    BUILD_BUG_ON(sizeof(xen_domain_handle_t) != 16);

Also, these should be declared at the beginning of the
function instead
of patched in to the middle by a sub-scope (this will also
fix the
indenting of the xc_ia64_build_hob() call below).

> +        uint32_t xen_version;
> +        char xen_extra_version[XEN_EXTRAVERSION_LEN];
> +
> +        hypercall_xen_version(xc_handle, dom,
XENVER_guest_handle,
> +                            (void *)uuid,
sizeof(uuid));
> +        hypercall_xen_version(xc_handle, dom,
XENVER_version,
> +                            (void *)&xen_version,
> sizeof(xen_version));
> +        hypercall_xen_version(xc_handle, dom,
XENVER_extraversion,
> +                            (void
*)xen_extra_version,
> sizeof(xen_extra_version));
> +
> +    // Hand-off state passed to guest firmware
> +    if (xc_ia64_build_hob(xc_handle, dom, dom_memsize,
vcpus,
> nvram_start,
> +                uuid, xen_version, xen_extra_version)
< 0) {
>          PERROR("Could not build hobn");
>          goto error_out;
>      }
>  
-- 
Alex Williamson                             HP Open Source
& Linux Org.


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel

Hi Alex:
	Thx your advice. When I modify the patch, I found
a problem that is hardly to keep away from. 
	I do a hypercall in setup_guest() to get UUID of XEN,
at this time the context belongs to DOM0. So the hypercall 
would return UUID info of dom0 ( it return
current->domain->handle) which is zero. I can't find
another 
place to put the codes because we need properly pass the
value
to GFW hob. 
	Do you have any advice? I want to add a hypercall interface

in GFW to do that.

Good good study,day day up ! ^_^
-Wing(zhang xin)

OTC,Intel Corporation
>-----Original Message-----
>From: Alex Williamson [mailto:alex.williamsonhp.com]
>Sent: 2007$BG/(J8$B7n(J28$BF|(J 5:42
>To: Zhang, Xing Z
>Cc: xen-ia64-devel
>Subject: RE: [Xen-ia64-devel][PATCH] SMBios table
support in
>XEN
>
>Hi,
>
>   I'll point out the specific place I was thinking
below, along
>with a
>couple other issues.  When I test this with the latest
GFW, I'm
>also
>getting a couple of these on domain startup:
>
>xencomm_privcmd_xen_version: unknown version op 17
>
>And I'm not seeing an SMBIOS table with dmidecode in the
guest.
>Is
>there still something missing?  Thanks,
>
>	Alex
>
>
>On Fri, 2007-08-24 at 13:11 +0800, Zhang, Xing Z wrote:
>> +/* Founctions for building SMBios table */
>> +static void hypercall_xen_version(int xc_handle,
domid_t
>dom, int
>> type, void *val, int len)
>> +{
>
>   I think this needs to be more strict in it's
"type"
>validation.
>Currently XENVER_guest_handle returns one thing,
anything at
>all passed
>to it returns a version.  The version stuff seems a
little
>redundant
>with xc_version().  Could we use that instead?
>
>> +    if ( type != XENVER_guest_handle )
>> +    {
>> +        DECLARE_HYPERCALL;
>> +
>> +        hypercall.op = __HYPERVISOR_xen_version;
>> +        hypercall.arg[0] =
__HYPERVISOR_xen_version;
>> +
>> +        lock_pages(val, len);
>> +        do_xen_hypercall(xc_handle,
&hypercall);
>> +        unlock_pages(val, len);
>> +
>> +        memcpy(val, &hypercall.arg[1], len);
>> +    }
>> +    else
>> +    {
>> +        DECLARE_DOMCTL;
>> +
>> +        domctl.cmd = XEN_DOMCTL_getdomaininfo;
>> +        domctl.domain = dom;
>> +
>> +        if (xc_domctl(xc_handle, &domctl) <
0)
>> +        {
>> +            PERROR("Get XEN UUID failed,
SMBios may be built
>wrong!
>> n");
>> +            memset(val, 0x0, len);
>> +            return;
>> +        }
>> +
>> +        memcpy(val, domctl.u.getdomaininfo.handle,
len);
>> +    }
>>  }
>...
>>  -1068,15 +1142,31  setup_guest(int xc_handle,
uint32_t
>dom,
>>
>>      vcpus = domctl.u.getdomaininfo.max_vcpu_id +
1;
>>
>> -    // Hand-off state passed to guest firmware
>> -    if (xc_ia64_build_hob(xc_handle, dom,
dom_memsize,
>vcpus,
>> nvram_start) < 0) {
>> +    // Get Xen version info for building SMBios
table
>> +    {
>> +        uint8_t uuid[XENVER_UUID_LEN]; // This
will break if
>> xen_domain_handle_t is not uint8_t[16]
>
>   This is where I thought maybe we should use a
>xen_domain_handle_t
>like Keir did in uptream:
>
>    xen_domain_handle_t uuid;
>
>    ...
>
>    BUILD_BUG_ON(sizeof(xen_domain_handle_t) != 16);
>
>Also, these should be declared at the beginning of the
function
>instead
>of patched in to the middle by a sub-scope (this will
also fix
>the
>indenting of the xc_ia64_build_hob() call below).
>
>> +        uint32_t xen_version;
>> +        char
xen_extra_version[XEN_EXTRAVERSION_LEN];
>> +
>> +        hypercall_xen_version(xc_handle, dom,
>XENVER_guest_handle,
>> +                            (void *)uuid,
sizeof(uuid));
>> +        hypercall_xen_version(xc_handle, dom,
>XENVER_version,
>> +                            (void
*)&xen_version,
>> sizeof(xen_version));
>> +        hypercall_xen_version(xc_handle, dom,
>XENVER_extraversion,
>> +                            (void
*)xen_extra_version,
>> sizeof(xen_extra_version));
>> +
>> +    // Hand-off state passed to guest firmware
>> +    if (xc_ia64_build_hob(xc_handle, dom,
dom_memsize,
>vcpus,
>> nvram_start,
>> +                uuid, xen_version,
xen_extra_version) < 0) {
>>          PERROR("Could not build
hobn");
>>          goto error_out;
>>      }
>>
>--
>Alex Williamson                             HP Open
Source & Linux
>Org.

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devellists.xensource.com
http://list
s.xensource.com/xen-ia64-devel