> -----Original Message-----
> From: Anthony PERARD <anthony.per...@citrix.com>
> Sent: 16 September 2019 15:06
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Jan Beulich <jbeul...@suse.com>;
> Christian Lindig
> <christian.lin...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Wei Liu
> <w...@xen.org>; Andrew
> Cooper <andrew.coop...@citrix.com>; George Dunlap <george.dun...@citrix.com>;
> Julien Grall
> <julien.gr...@arm.com>; Konrad Rzeszutek Wilk <konrad.w...@oracle.com>;
> Stefano Stabellini
> <sstabell...@kernel.org>; Tim (Xen.org) <t...@xen.org>; David Scott
> <d...@recoil.org>; Volodymyr
> Babchuk <volodymyr_babc...@epam.com>; Roger Pau Monne <roger....@citrix.com>
> Subject: Re: [PATCH v12 6/6] introduce a 'passthrough' configuration option
> to xl.cfg...
>
> On Mon, Sep 16, 2019 at 10:27:08AM +0100, Paul Durrant wrote:
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index 59dbcb50a0..7afae81432 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -30,6 +30,12 @@
> > int libxl__domain_create_info_setdefault(libxl__gc *gc,
> > libxl_domain_create_info *c_info)
> > {
> > + libxl_physinfo info;
> > + int rc = libxl_get_physinfo(CTX, &info);
> > +
> > + if (rc)
> > + return rc;
> > +
>
> I think this hunk would be more readable if it was written:
> int rc;
>
> rc = libxl_get_physinfo(CTX, &info);
> if (rc)
> return rc;
>
> Otherwise, the check for error is alone which might mean "we are in a
> callback function and check for rc passed by parameter" or that it's a
> stray check.
>
> > @@ -62,6 +62,12 @@ int libxl__domain_create_info_setdefault(libxl__gc *gc,
> > if (!c_info->ssidref)
> > c_info->ssidref = SECINITSID_DOMU;
> >
> > + if (info.cap_hvm_directio) {
> > + c_info->passthrough = ((c_info->type == LIBXL_DOMAIN_TYPE_PV) ||
> > + !info.cap_iommu_hap_pt_share) ?
> > + LIBXL_PASSTHROUGH_SYNC_PT : LIBXL_PASSTHROUGH_SHARE_PT;
> > + }
>
> I don't think you can do that. This will overwrite the value that was in
> c_info before (that a user as set). The _setdefault() function is only
> called after c_info have been filled by users of the libxl, it only has
> to change the value if it was the default.
Oh, what a useful semantic.
> c_info->passthrough has no
> default value, so it's not possible to know what a user wants.
>
> What about adding "default"==0 in libxl_passthrough enum?
I guess that's probably the only option, but there is still the problem of the
page table memory overhead. I guess I'd have to assume the worst case for
'default'. That separation between create info and build info is really
unhelpful.
>
> > return 0;
> > }
> >
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index d52c63b6b0..22f05711e3 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -263,6 +263,12 @@ libxl_vkb_backend = Enumeration("vkb_backend", [
> > (2, "LINUX")
> > ])
> >
> > +libxl_passthrough = Enumeration("passthrough", [
> > + (0, "disabled"),
> > + (1, "sync_pt"),
> > + (2, "share_pt"),
> > + ])
> > +
> > #
> > # Complex libxl types
> > #
> > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> > index 293f5f730e..4b2baa0403 100644
> > --- a/tools/xl/xl_parse.c
> > +++ b/tools/xl/xl_parse.c
> > @@ -1461,6 +1461,107 @@ void parse_config_data(const char *config_source,
> > + if (xlu_cfg_get_string(config, "passthrough", &buf, 0)) {
> > + buf = (d_config->num_pcidevs || d_config->num_dtdevs)
> > + ? "enabled" : "disabled";
> > + }
> > +
> > + if (!strncmp(buf, "enabled", strlen(buf))) {
>
> Do you intend to have "passthrough=''" been the equivalent of
> "passthrough='enabled'" in the config file ?
> (same with "e", "en", "ena", ...)
No, I guess not; I'll fix that.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
