Re: [Xen-devel] [PATCH v8 3/4] libxl: attach PCI device to qemu only after setting pciback/pcifront

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Sat, Sep 28, 2019 at 04:20:36PM +0200, Marek Marczykowski-Górecki wrote:
> When qemu is running in stubdomain, handling "pci-ins" command will fail
> if pcifront is not initialized already. Fix this by sending such command
> only after confirming that pciback/front is running.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - Fixed code style since previous version.
> Changes in v8:
> - rebase on staging
> - rework for async api

The patch looks good, I only have coding style comments. With those
addressed:
Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>


> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index ac597a5..ba0287d 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -1487,6 +1490,10 @@ static int libxl_pcidev_assignable(libxl_ctx *ctx, 
> libxl_device_pci *pcidev)
>      return i != num;
>  }
>  
> +static void device_pci_add_stubdom_wait(libxl__egc *egc,
> +    pci_add_state *pas, int rc);
> +static void device_pci_add_stubdom_ready(libxl__egc *egc,
> +    libxl__ev_devstate* ds, int rc);

s/* ds/ *ds/

>  static void device_pci_add_stubdom_done(libxl__egc *egc,
>      pci_add_state *, int rc);
>  static void device_pci_add_done(libxl__egc *egc,
> @@ -1575,6 +1583,41 @@ out:
>      device_pci_add_done(egc, pas, rc); /* must be last */
>  }
>  
> +static void device_pci_add_stubdom_wait(libxl__egc *egc,
> +                                        pci_add_state *pas,
> +                                        int rc)
> +{
> +    libxl__ao_device *aodev = pas->aodev;
> +    STATE_AO_GC(aodev->ao);
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    int stubdomid = libxl_get_stubdom_id(ctx, pas->domid);

You can use `CTX' instead of declaring a local `ctx'

> +    char *state_path;
> +
> +    if (rc) goto out;
> +
> +    /* Wait for the device actually being connected, otherwise device model
> +     * running there will fail to find the device. */
> +    state_path = libxl__sprintf(gc, "%s/backend/pci/%d/0/state",
> +            libxl__xs_get_dompath(gc, 0), stubdomid);

I think you could use libxl__domain_device_backend_path(gc, 0,
stubdomid, 0, LIBXL__DEVICE_KIND_PCI) here instead.
Or at least "GCSPRINTF(" instead of "libxl__sprintf(gc, "

> +    rc = libxl__ev_devstate_wait(aodev->ao, &pas->pciback_ds,

You can use `ao' instead of `aodev->ao'. `ao' is decleared by
STATE_AO_GC().

> +            device_pci_add_stubdom_ready,
> +            state_path, XenbusStateConnected,
> +            LIBXL_DEVICE_MODEL_START_TIMEOUT * 1000);

Thanks,

-- 
Anthony PERARD

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