RE: [Xen-ia64-devel][PATCH] SMBios table support in XEN

[Alex Williamson <alex.williamson@xxxxxx>]

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 hob\n");
>          goto error_out;
>      }
>  
-- 
Alex Williamson                             HP Open Source & Linux Org.


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel