Re: [Xen-devel] [PATCH v12 6/6] introduce a 'passthrough' configuration option to xl.cfg...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Sent: 16 September 2019 15:06
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Jan Beulich <jbeulich@xxxxxxxx>; 
> Christian Lindig
> <christian.lindig@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Andrew
> Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; 
> Julien Grall
> <julien.grall@xxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; 
> Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; David Scott 
> <dave@xxxxxxxxxx>; Volodymyr
> Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel