From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH 1/5] libxl: introduce cpuid interface to domain build Date: Thu, 9 Sep 2010 21:16:11 +0200 Message-ID: <4C89327B.30703@amd.com> References: <4C87551F.4080300@amd.com> <1283939221.14311.1477.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------010800000409050505050806" Return-path: In-Reply-To: <1283939221.14311.1477.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Campbell Cc: "xen-devel@lists.xensource.com" , Keir Fraser , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org --------------010800000409050505050806 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit Ian Campbell wrote: > On Wed, 2010-09-08 at 10:19 +0100, Andre Przywara wrote: >> Add a cpuid parameter into libxl_domain_build_info and use >> it's content while setting up the domain. This is a only paving the way, >> the real functionality is implemented in a later patch. >> >> Signed-off-by: Andre Przywara > > The destructor function should free the type but not the reference to > it, so: > >> @@ -102,6 +102,21 @@ void >> libxl_key_value_list_destroy(libxl_key_value_list *pkvl) >> free(kvl); >> } >> >> +void libxl_cpuid_destroy(libxl_cpuid_policy_list cpuid_list) > > This should be *cpuid_list and the function should be adjusted to free > the elements of *cpuid_list but not cpuid_list itself. > >> --- a/tools/libxl/libxl.idl >> +++ b/tools/libxl/libxl.idl >> @@ -11,6 +11,7 @@ libxl_console_consback = Builtin("console_consback") >> libxl_console_constype = Builtin("console_constype") >> libxl_disk_phystype = Builtin("disk_phystype") >> libxl_nic_type = Builtin("nic_type") >> +libxl_cpuid_policy_list = Builtin("cpuid_policy_list", destructor_fn="libxl_cpuid_destroy") > > And this should have passby=PASS_BY_REFERENCE. > > See 22078:573ddf5cc145 for a similar change to the libxl_string_list and > libxl_key_value_list destructor functions. Do you mean like in the attached patch? > >> --- a/tools/libxl/libxl.h >> +++ b/tools/libxl/libxl.h >> @@ -172,6 +172,22 @@ typedef enum { >> NICTYPE_VIF, >> } libxl_nic_type; >> >> +/* holds the CPUID response for a single CPUID leaf >> + * input contains the value of the EAX and ECX register, >> + * and each policy string contains a filter to apply to >> + * the host given values for that particular leaf. >> + */ >> +struct libxl_cpuid_policy { >> + uint32_t input[2]; >> + char *policy[4]; >> +}; > > I realise (at least I think I do) that this is just exposing the > existing hypervisor/libxc interface warts and all but would this be more > obvious to users if it was: > struct libxl_cpuid_policy { > uint32_t eax; > uint32_t ecx; > > char *eax_filter; > char *ebx_filter; > char *ecx_filter; > char *edx_filter; > } > > could either translate in libxl or push the change down into libxc. Actually I consider this structure not a real interface, but more an opaque kludge to transport the data from the configuration parsing to domain creation. If you want to change the data here, I'd like to see the parse functions used. Actually I designed them such that one can alter the policy at any time by chaining calls to these functions. This is my plan to use in the upcoming multi-core patch, it would simply call libxl_cpuid_parse_config(&b_info->cpuid, "proccount=4"); to make it ultimately readable. Beside that I'd rather hide the dynamic array nature of it. > Alternatively > #define LIBXL_CPUID_INPUT_EAX 0 > #define LIBXL_CPUID_INPUT_ECX 1 > > #define LIBXL_CPUID_FILTER_EAX 0 > #define LIBXL_CPUID_FILTER_EBX 1 > ... > would at least make the code (or at least the data structure) a bit more > obvious. I am not sure whether that would help. The interface is too error-prone to be directly used by other functions than those parsers, so I'd like not to promote it with defining macros (which I probably wouldn't use in my own code, since I mostly either propagate the reg number or enumerate over all registers). If you like I could introduce a kind of low-level function, like: libxl_cpuid_set_policy(libxl_cpuid_policy_list *list, uint32_t leaf, int bit, char policy); That could be used by other parts of libxl as well and would care about the string fiddling and allocation. Do we need this function? Shall I state the opaque nature of this structure in a comment? Regards, Andre. -- Andre Przywara AMD-Operating System Research Center (OSRC), Dresden, Germany Tel: +49 351 448-3567-12 --------------010800000409050505050806 Content-Type: text/plain; name="0001-libxl-introduce-cpuid-interface-to-domain-build_v2.patch" Content-Transfer-Encoding: base64 Content-Disposition: inline; filename*0="0001-libxl-introduce-cpuid-interface-to-domain-build_v2.patc"; filename*1="h" Y29tbWl0IDM1MGNlNjFjM2M5NmE1ODJhNmY1ZDY0MWY0ZmZjMjBlMDgwNjE4MmEKQXV0aG9y OiBBbmRyZSBQcnp5d2FyYSA8YW5kcmUucHJ6eXdhcmFAYW1kLmNvbT4KRGF0ZTogICBUdWUg QXVnIDI0IDA5OjM1OjUxIDIwMTAgKzAyMDAKCiAgICB4bDogaW50cm9kdWNlIGNwdWlkIGlu dGVyZmFjZSB0byBkb21haW4gYnVpbGQKICAgIAogICAgdGhpcyBvbmUgYWRkcyBhIGNwdWlk IHBhcmFtZXRlciBpbnRvIGxpYnhsX2RvbWFpbl9idWlsZF9pbmZvIGFuZCB1c2VzCiAgICBp dCdzIGNvbnRlbnQgd2hpbGUgc2V0dGluZyB1cCB0aGUgZG9tYWluLiBUaGlzIGlzIGEgcGxh Y2Vob2xkZXIgZm9yCiAgICBub3csIHNpbmNlIHRoZSBwYXJzaW5nIGlzIG9ubHkgaW1wbGVt ZW50ZWQgaW4gdGhlIG5leHQgcGF0Y2guCiAgICAKICAgIFNpZ25lZC1vZmYtYnk6IEFuZHJl IFByenl3YXJhIDxhbmRyZS5wcnp5d2FyYUBhbWQuY29tPgoKZGlmZiAtLWdpdCBhL3Rvb2xz L2xpYnhsL2xpYnhsLmMgYi90b29scy9saWJ4bC9saWJ4bC5jCmluZGV4IDAzZDlhOTMuLjU2 NGFmYzYgMTAwNjQ0Ci0tLSBhL3Rvb2xzL2xpYnhsL2xpYnhsLmMKKysrIGIvdG9vbHMvbGli eGwvbGlieGwuYwpAQCAtMTAyLDYgKzEwMiwyMSBAQCB2b2lkIGxpYnhsX2tleV92YWx1ZV9s aXN0X2Rlc3Ryb3kobGlieGxfa2V5X3ZhbHVlX2xpc3QgKnBrdmwpCiAgICAgZnJlZShrdmwp OwogfQogCit2b2lkIGxpYnhsX2NwdWlkX2Rlc3Ryb3kobGlieGxfY3B1aWRfcG9saWN5X2xp c3QgKnBfY3B1aWRfbGlzdCkKK3sKKyAgICBpbnQgaSwgajsKKyAgICBsaWJ4bF9jcHVpZF9w b2xpY3lfbGlzdCBjcHVpZF9saXN0ID0gKnBfY3B1aWRfbGlzdDsKKworICAgIGlmIChjcHVp ZF9saXN0ID09IE5VTEwpCisgICAgICAgIHJldHVybjsKKyAgICBmb3IgKGkgPSAwOyBjcHVp ZF9saXN0W2ldLmlucHV0WzBdICE9IFhFTl9DUFVJRF9JTlBVVF9VTlVTRUQ7IGkrKykgewor ICAgICAgICBmb3IgKGogPSAwOyBqIDwgNDsgaisrKQorICAgICAgICAgICAgaWYgKGNwdWlk X2xpc3RbaV0ucG9saWN5W2pdICE9IE5VTEwpCisgICAgICAgICAgICAgICAgZnJlZShjcHVp ZF9saXN0W2ldLnBvbGljeVtqXSk7CisgICAgfQorICAgIHJldHVybjsKK30KKwogLyoqKioq KioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioqKioq KioqKioqKioqKioqKioqKioqKi8KIAogaW50IGxpYnhsX2RvbWFpbl9tYWtlKGxpYnhsX2N0 eCAqY3R4LCBsaWJ4bF9kb21haW5fY3JlYXRlX2luZm8gKmluZm8sCmRpZmYgLS1naXQgYS90 b29scy9saWJ4bC9saWJ4bC5oIGIvdG9vbHMvbGlieGwvbGlieGwuaAppbmRleCBkOTg5ZjEw Li41MGM5ZTNkIDEwMDY0NAotLS0gYS90b29scy9saWJ4bC9saWJ4bC5oCisrKyBiL3Rvb2xz L2xpYnhsL2xpYnhsLmgKQEAgLTE3Miw2ICsxNzIsMjIgQEAgdHlwZWRlZiBlbnVtIHsKICAg ICBOSUNUWVBFX1ZJRiwKIH0gbGlieGxfbmljX3R5cGU7CiAKKy8qIGhvbGRzIHRoZSBDUFVJ RCByZXNwb25zZSBmb3IgYSBzaW5nbGUgQ1BVSUQgbGVhZgorICogaW5wdXQgY29udGFpbnMg dGhlIHZhbHVlIG9mIHRoZSBFQVggYW5kIEVDWCByZWdpc3RlciwKKyAqIGFuZCBlYWNoIHBv bGljeSBzdHJpbmcgY29udGFpbnMgYSBmaWx0ZXIgdG8gYXBwbHkgdG8KKyAqIHRoZSBob3N0 IGdpdmVuIHZhbHVlcyBmb3IgdGhhdCBwYXJ0aWN1bGFyIGxlYWYuCisgKi8gCitzdHJ1Y3Qg bGlieGxfY3B1aWRfcG9saWN5IHsKKyAgICB1aW50MzJfdCBpbnB1dFsyXTsKKyAgICBjaGFy ICpwb2xpY3lbNF07Cit9OworCisvKiBsaWJ4bF9jcHVpZF9wb2xpY3lfbGlzdCBpcyBhIGR5 bmFtaWMgYXJyYXkgc3RvcmluZyBDUFVJRCBwb2xpY2llcworICogZm9yIG11bHRpcGxlIGxl YWZzLiBJdCBpcyB0ZXJtaW5hdGVkIHdpdGggYW4gZW50cnkgaG9sZGluZworICogWEVOX0NQ VUlEX0lOUFVUX1VOVVNFRCBpbiBpbnB1dFswXQorICovCit0eXBlZGVmIHN0cnVjdCBsaWJ4 bF9jcHVpZF9wb2xpY3kgKiBsaWJ4bF9jcHVpZF9wb2xpY3lfbGlzdDsKKwogI2RlZmluZSBM SUJYTF9QQ0lfRlVOQ19BTEwgKH4wVSkKIAogI2luY2x1ZGUgIl9saWJ4bF90eXBlcy5oIgpA QCAtMjMxLDYgKzI0Nyw3IEBAIGludCBsaWJ4bF9kb21haW5fcHJlc2VydmUobGlieGxfY3R4 ICpjdHgsIHVpbnQzMl90IGRvbWlkLCBsaWJ4bF9kb21haW5fY3JlYXRlX2luCiB2b2lkIGxp YnhsX3N0cmluZ19saXN0X2Rlc3Ryb3kobGlieGxfc3RyaW5nX2xpc3QgKnNsKTsKIHZvaWQg bGlieGxfa2V5X3ZhbHVlX2xpc3RfZGVzdHJveShsaWJ4bF9rZXlfdmFsdWVfbGlzdCAqa3Zs KTsKIHZvaWQgbGlieGxfZmlsZV9yZWZlcmVuY2VfZGVzdHJveShsaWJ4bF9maWxlX3JlZmVy ZW5jZSAqZik7Cit2b2lkIGxpYnhsX2NwdWlkX2Rlc3Ryb3kobGlieGxfY3B1aWRfcG9saWN5 X2xpc3QgKmNwdWlkX2xpc3QpOwogCiAvKgogICogUnVuIHRoZSBjb25maWd1cmVkIGJvb3Rs b2FkZXIgZm9yIGEgUFYgZG9tYWluIGFuZCB1cGRhdGUKZGlmZiAtLWdpdCBhL3Rvb2xzL2xp YnhsL2xpYnhsLmlkbCBiL3Rvb2xzL2xpYnhsL2xpYnhsLmlkbAppbmRleCAxZTM2OTI2Li5k YTdmYzdhIDEwMDY0NAotLS0gYS90b29scy9saWJ4bC9saWJ4bC5pZGwKKysrIGIvdG9vbHMv bGlieGwvbGlieGwuaWRsCkBAIC0xMSw2ICsxMSw3IEBAIGxpYnhsX2NvbnNvbGVfY29uc2Jh Y2sgPSBCdWlsdGluKCJjb25zb2xlX2NvbnNiYWNrIikKIGxpYnhsX2NvbnNvbGVfY29uc3R5 cGUgPSBCdWlsdGluKCJjb25zb2xlX2NvbnN0eXBlIikKIGxpYnhsX2Rpc2tfcGh5c3R5cGUg PSBCdWlsdGluKCJkaXNrX3BoeXN0eXBlIikKIGxpYnhsX25pY190eXBlID0gQnVpbHRpbigi bmljX3R5cGUiKQorbGlieGxfY3B1aWRfcG9saWN5X2xpc3QgPSBCdWlsdGluKCJjcHVpZF9w b2xpY3lfbGlzdCIsIGRlc3RydWN0b3JfZm49ImxpYnhsX2NwdWlkX2Rlc3Ryb3kiLCBwYXNz Ynk9UEFTU19CWV9SRUZFUkVOQ0UpCiAKIGxpYnhsX3N0cmluZ19saXN0ID0gQnVpbHRpbigi c3RyaW5nX2xpc3QiLCBkZXN0cnVjdG9yX2ZuPSJsaWJ4bF9zdHJpbmdfbGlzdF9kZXN0cm95 IiwgcGFzc2J5PVBBU1NfQllfUkVGRVJFTkNFKQogbGlieGxfa2V5X3ZhbHVlX2xpc3QgPSBC dWlsdGluKCJrZXlfdmFsdWVfbGlzdCIsIGRlc3RydWN0b3JfZm49ImxpYnhsX2tleV92YWx1 ZV9saXN0X2Rlc3Ryb3kiLCBwYXNzYnk9UEFTU19CWV9SRUZFUkVOQ0UpCkBAIC05Nyw2ICs5 OCw3IEBAIGxpYnhsX2RvbWFpbl9idWlsZF9pbmZvID0gU3RydWN0KCJkb21haW5fYnVpbGRf aW5mbyIsWwogICAgICgic2hhZG93X21lbWtiIiwgICAgdWludDMyKSwKICAgICAoImRpc2Fi bGVfbWlncmF0ZSIsIGJvb2wpLAogICAgICgia2VybmVsIiwgICAgICAgICAgbGlieGxfZmls ZV9yZWZlcmVuY2UpLAorICAgICgiY3B1aWQiLCAgICAgICAgICAgbGlieGxfY3B1aWRfcG9s aWN5X2xpc3QpLAogICAgICgiaHZtIiwgICAgICAgICAgICAgaW50ZWdlciksCiAgICAgKCJ1 IiwgS2V5ZWRVbmlvbihOb25lLCAiaHZtIiwKICAgICAgICAgICAgICAgICBbKCJodm0iLCAi JXMiLCBTdHJ1Y3QoTm9uZSwKZGlmZiAtLWdpdCBhL3Rvb2xzL2xpYnhsL2xpYnhsX2RvbS5j IGIvdG9vbHMvbGlieGwvbGlieGxfZG9tLmMKaW5kZXggZTgzYmJiZi4uZThjMGJkYSAxMDA2 NDQKLS0tIGEvdG9vbHMvbGlieGwvbGlieGxfZG9tLmMKKysrIGIvdG9vbHMvbGlieGwvbGli eGxfZG9tLmMKQEAgLTkxLDkgKzkxLDE1IEBAIGludCBidWlsZF9wb3N0KGxpYnhsX2N0eCAq Y3R4LCB1aW50MzJfdCBkb21pZCwKICAgICB4c190cmFuc2FjdGlvbl90IHQ7CiAgICAgY2hh ciAqKmVudHM7CiAgICAgaW50IGk7CisgICAgY2hhciAqY3B1aWRfcmVzWzRdOwogCiAjaWYg ZGVmaW5lZChfX2kzODZfXykgfHwgZGVmaW5lZChfX3g4Nl82NF9fKQogICAgIHhjX2NwdWlk X2FwcGx5X3BvbGljeShjdHgtPnhjaCwgZG9taWQpOworICAgIGlmIChpbmZvLT5jcHVpZCAh PSBOVUxMKSB7CisgICAgICAgIGZvciAoaSA9IDA7IGluZm8tPmNwdWlkW2ldLmlucHV0WzBd ICE9IFhFTl9DUFVJRF9JTlBVVF9VTlVTRUQ7IGkrKykKKyAgICAgICAgICAgIHhjX2NwdWlk X3NldChjdHgtPnhjaCwgZG9taWQsIGluZm8tPmNwdWlkW2ldLmlucHV0LAorICAgICAgICAg ICAgICAgICAgICAgICAgIChjb25zdCBjaGFyKiopKGluZm8tPmNwdWlkW2ldLnBvbGljeSks IGNwdWlkX3Jlcyk7CisgICAgfQogI2VuZGlmCiAKICAgICBlbnRzID0gbGlieGxfY2FsbG9j KCZnYywgMTIgKyAoaW5mby0+bWF4X3ZjcHVzICogMikgKyAyLCBzaXplb2YoY2hhciAqKSk7 CmRpZmYgLS1naXQgYS90b29scy9saWJ4bC94bF9jbWRpbXBsLmMgYi90b29scy9saWJ4bC94 bF9jbWRpbXBsLmMKaW5kZXggM2Y2MjE5Yi4uYzZiNmQ4NSAxMDA2NDQKLS0tIGEvdG9vbHMv bGlieGwveGxfY21kaW1wbC5jCisrKyBiL3Rvb2xzL2xpYnhsL3hsX2NtZGltcGwuYwpAQCAt MjY4LDYgKzI2OCw3IEBAIHN0YXRpYyB2b2lkIGluaXRfYnVpbGRfaW5mbyhsaWJ4bF9kb21h aW5fYnVpbGRfaW5mbyAqYl9pbmZvLCBsaWJ4bF9kb21haW5fY3JlYXRlCiAgICAgYl9pbmZv LT5tYXhfbWVta2IgPSAzMiAqIDEwMjQ7CiAgICAgYl9pbmZvLT50YXJnZXRfbWVta2IgPSBi X2luZm8tPm1heF9tZW1rYjsKICAgICBiX2luZm8tPmRpc2FibGVfbWlncmF0ZSA9IDA7Cisg ICAgYl9pbmZvLT5jcHVpZCA9IE5VTEw7CiAgICAgaWYgKGNfaW5mby0+aHZtKSB7CiAgICAg ICAgIGJfaW5mby0+c2hhZG93X21lbWtiID0gMDsgLyogU2V0IGxhdGVyICovCiAgICAgICAg IGJfaW5mby0+dmlkZW9fbWVta2IgPSA4ICogMTAyNDsK --------------010800000409050505050806 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --------------010800000409050505050806--