Re: [PATCH 2/6] xl: Add commands for hiding and unhiding pcie passthrough devices

[Venu Busireddy <venu.busireddy@oracle.com>]

On 2017-06-30 11:18:10 +0100, Wei Liu wrote:
> I haven't reviewed the code in detail, but I have some questions
> regarding the design. See the end of this email.
> 
> On Tue, Jun 27, 2017 at 12:14:54PM -0500, Venu Busireddy wrote:
> >  
> > +static void pciassignable_list_hidden(void)
> > +{
> > +    libxl_device_pci *pcidevs;
> > +    int num, i;
> > +
> > +    pcidevs = libxl_device_pci_assignable_list(ctx, &num);
> > +
> > +    if ( pcidevs == NULL )
> 
> Coding style.

Will fix.

> > +        return;
> > +    for (i = 0; i < num; i++) {
> > +        if (libxl_device_pci_assignable_is_hidden(ctx, &pcidevs[i]))
> > +            printf("%04x:%02x:%02x.%01x\n",
> > +                   pcidevs[i].domain, pcidevs[i].bus, pcidevs[i].dev, pcidevs[i].func);
> > +        libxl_device_pci_dispose(&pcidevs[i]);
> > +    }
> > +    free(pcidevs);
> > +}
> > +
> > +int main_pciassignable_list_hidden(int argc, char **argv)
> > +{
> > +    int opt;
> > +
> > +    SWITCH_FOREACH_OPT(opt, "", NULL, "pci-assignable-list-hidden", 0) {
> > +        /* No options */
> > +    }
> > +
> > +    pciassignable_list_hidden();
> > +    return 0;
> > +}
> > +
> > +static int pciassignable_hide(const char *bdf)
> > +{
> > +    libxl_device_pci pcidev;
> > +    XLU_Config *config;
> > +    int r = EXIT_SUCCESS;
> > +
> > +    libxl_device_pci_init(&pcidev);
> > +
> > +    config = xlu_cfg_init(stderr, "command line");
> > +    if (!config) {
> > +        perror("xlu_cfg_init");
> > +        exit(-1);
> 
> If you don't want EXIT_FAILURE, please document these exit values
> somewhere -- manpage would be a good place.

I was following the semantics that other similar functions in that file
(such as pciassignable_add(), etc.) were following, and hence the exit
value of '-1'. I will change this to exit with EXIT_FAILURE.

> > +    }
> > +
> > +    if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
> > +        fprintf(stderr, "pci-assignable-hide: malformed BDF specification \"%s\"\n", bdf);
> > +        exit(2);
> > +    }
> > +
> > +    if (libxl_device_pci_assignable_hide(ctx, &pcidev))
> > +        r = EXIT_FAILURE;
> > +
> > +    libxl_device_pci_dispose(&pcidev);
> > +    xlu_cfg_destroy(config);
> > +
> > +    return r;
> > +}
> > +
> > +int main_pciassignable_hide(int argc, char **argv)
> > +{
> > +    int opt;
> > +    const char *bdf = NULL;
> > +
> > +    SWITCH_FOREACH_OPT(opt, "", NULL, "main_pciassignable_hide", 1) {
> > +        /* No options */
> > +    }
> > +
> > +    bdf = argv[optind];
> > +
> > +    if (pciassignable_hide(bdf))
> > +        return EXIT_FAILURE;
> > +
> > +    return EXIT_SUCCESS;
> > +}
> [...]
> > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> > index 89c2b25..10a48a9 100644
> > --- a/tools/xl/xl_vmcontrol.c
> > +++ b/tools/xl/xl_vmcontrol.c
> > @@ -966,6 +966,15 @@ start:
> >      LOG("Waiting for domain %s (domid %u) to die [pid %ld]",
> >          d_config.c_info.name, domid, (long)getpid());
> >  
> > +    ret = libxl_reg_aer_events_handler(ctx, domid);
> > +    if (ret) {
> > +        /*
> > +         * This error may not be severe enough to fail the creation of the VM.
> > +         * Log the error, and continue with the creation.
> > +         */
> > +        LOG("libxl_reg_aer_events_handler() failed, ret = 0x%08x", ret);
> > +    }
> > +
> 
> First thing this suggests the ordering of this patch series is wrong --
> you need to put the patch that implements the new function before this.

I will change the order in the next revision.

> The other thing you need to be aware is that if the user chooses to not
> use a daemonised xl, he / she doesn't get a chance to handle these
> events.
> 
> This is potentially problematic for driver domains. You probably want to
> also modify xl devd command. Also on the subject, what's your thought on
> driver domain? I'm not sure if a driver domain has the permission to
> kill the guest.

I don't know if I understood your question correctly, but it is not the
driver domain that is killing another guest. It is Dom0 that is killing
the guest to which the device is assigned in passthrough mode. That guest
should still be killable by Dom0, even if it is a driver domain. Right?

However, I have been asked by Jan Beulich (and many others) on the
need to kill the guest, and why the device can't be unassigned from
that guest! My initial thinking (for the first revision) was that the
guest and the device together are party to evil things, and hence the
guest should be killed. But I agree that unassigning the device should
be sufficient. Once the device is removed, the guest can't do much that
any other guest can't. Therefore, I plan to change this patchset to
simply unassign the device from the guest. This aspect is also covered
in the thread:

https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg00552.html

May I request you review that thread and post your thoughts?

And if we go with that approach, some of the questions related to
hide/unhide operations will be obviated!

Thanks,

Venu


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel