Re: [Xen-devel] [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul passthrough setting logic

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Andrew Cooper writes ("Re: [XEN PATCH for-4.13 v2 9/9] libxl/xl: Overhaul 
passthrough setting logic"):
> On 10/10/2019 16:11, Ian Jackson wrote:
> > +    if (c_info->passthrough == LIBXL_PASSTHROUGH_DISABLED && need_pt) {
> > +        LOGD(ERROR, domid,
> > +             "passthrough disabled but devices are specified");
> 
> This is the only log message which isn't prefixed with ERROR:

I will strip the ERROR: out of the others.  I think I inherited them
from when this code was in xl, where it's just fprintf stderr.  Here
the priority is an argument to LOGD.

> > +    const char *whynot_pt_share =
> > +        c_info->type == LIBXL_DOMAIN_TYPE_PV ? "not valid for PV domain" :
> > +        !physinfo.cap_iommu_hap_pt_share ? "not supported on this 
> > platform" :
> > +        NULL;
> 
> This is a little more complicated.

I aimed to replicate the logic prior to my series.  FTAOD I think this
means this was already broken in xl ?  Anyway:

> For ARM, doesn't libxl treat guests as PV, or has that been fixed now? 
> ARM's only passthrough mode is PT_SHARE.

I think this means that I need to move the calculation of
whynot_pt_share into arch-specific code.  I'll wait and see what ARM
folks say.

> On x86 for PVH, passthrough doesn't work yet.  This may not be an
> argument against constructing the guest suitably, but we should check
> that things don't explode in new and interesting ways from this change.

If we know it doesn't work, it's not a good idea to accept it.  I will
arrange to reject it.

> For x86 HVM, PT_SHARE is only available for HAP guests, so shadow guests
> must use PT_SYNC.

I will add a check for c_info->hap.

> > +    /* An explicit setting should now have been chosen */
> > +    assert(c_info->passthrough != LIBXL_PASSTHROUGH_UNKNOWN);
> > +    assert(c_info->passthrough != LIBXL_PASSTHROUGH_ENABLED);
> 
> This is confusing.  I think it would help if ...
...
> ... this had a comment explaining enabled is just interim value.
> 
> (2, "enabled"), # becomes {sync,share}_pt once defaults are evaluated

Good idea, thanks.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel