Re: [Xen-devel] [PATCH v3 3/6] libxl: don't try to manipulate json config for stubdomain

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Jan 28, 2019 at 02:41:15PM +0000, Wei Liu wrote:
> On Sat, Jan 26, 2019 at 03:31:14AM +0100, Marek Marczykowski-Górecki wrote:
> > Stubdomain do not have it's own config file - its configuration is
> > derived from target domains. Do not try to manipulate it when attaching
> > PCI device.
> > 
> 
> So if we add the same configuration to stubdom as well, what would
> happen? I guess libxl will try to attach the PCI devices to both the
> stubdom and DomU?

I'm not sure if I understand you here. You mean adding configuration file for
stubdomain, the one managed by
libxl__get_domain_configuration/libxl__set_domain_configuration? In
theory it would work just fine, but in practice I fear all kind of
desynchronization bugs, like adding device to target domain's config,
but not stubdomain's one in some failure handling case. We'd have 4
things to care about:
 - attaching device to target domain
 - attaching device to stubdomain
 - saving device to target domain's config
 - saving device to stubdomain's config

Handling all the failure cases properly will become quite complex,
especially if we add async callbacks into the mix.
Since stubdomain config is deterministically build based on target
domian's config, I don't think adding such complexity is a good idea.

> > This bug prevented starting HVM with stubdomain and PCI passthrough
> > device attached.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > Changes in v3:
> >  - skip libxl__dm_check_start too, as stubdomain is guaranteed to be
> >    running at this stage already
> >  - do not init d_config at all, as it is used only for json manipulation
> > ---
> >  tools/libxl/libxl_pci.c | 48 ++++++++++++++++++++++++++----------------
> >  1 file changed, 30 insertions(+), 18 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index 1bde537..8d159cf 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -120,10 +120,14 @@ static int libxl__device_pci_add_xenstore(libxl__gc 
> > *gc, uint32_t domid, libxl_d
> >      libxl_domain_config d_config;
> >      libxl_device_pci pcidev_saved;
> >      libxl__domain_userdata_lock *lock = NULL;
> > +    bool is_stubdomain = libxl_is_stubdom(CTX, domid, NULL);
> >  
> > -    libxl_domain_config_init(&d_config);
> > -    libxl_device_pci_init(&pcidev_saved);
> > -    libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
> > +    /* Stubdomain doesn't have own config. */
> > +    if (!is_stubdomain) {
> > +        libxl_domain_config_init(&d_config);
> > +        libxl_device_pci_init(&pcidev_saved);
> > +        libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
> > +    }
> >  
> >      be_path = libxl__domain_device_backend_path(gc, 0, domid, 0,
> >                                                  LIBXL__DEVICE_KIND_PCI);
> > @@ -152,27 +156,33 @@ static int libxl__device_pci_add_xenstore(libxl__gc 
> > *gc, uint32_t domid, libxl_d
> >      GCNEW(device);
> >      libxl__device_from_pcidev(gc, domid, pcidev, device);
> >  
> > -    lock = libxl__lock_domain_userdata(gc, domid);
> > -    if (!lock) {
> > -        rc = ERROR_LOCK_FAIL;
> > -        goto out;
> > -    }
> > +    /* Stubdomain config is derived from its target domain, it doesn't have
> > +       its own file */
> 
> Although comment style isn't specified in CODING_STYLE, I would like to
> fix this to 
> 
>     /*
>      * Stubdom ...
>      * ... own file.
>      */

Ok.

> > +    if (!is_stubdomain) {
> > +        lock = libxl__lock_domain_userdata(gc, domid);
> > +        if (!lock) {
> > +            rc = ERROR_LOCK_FAIL;
> > +            goto out;
> > +        }
> >  
> > -    rc = libxl__get_domain_configuration(gc, domid, &d_config);
> > -    if (rc) goto out;
> > +        rc = libxl__get_domain_configuration(gc, domid, &d_config);
> > +        if (rc) goto out;
> >  
> > -    device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
> > -                             &pcidev_saved);
> > +        device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
> > +                                 &pcidev_saved);
> >  
> > -    rc = libxl__dm_check_start(gc, &d_config, domid);
> > -    if (rc) goto out;
> > +        rc = libxl__dm_check_start(gc, &d_config, domid);
> > +        if (rc) goto out;
> > +    }
> >  
> >      for (;;) {
> >          rc = libxl__xs_transaction_start(gc, &t);
> >          if (rc) goto out;
> >  
> > -        rc = libxl__set_domain_configuration(gc, domid, &d_config);
> > -        if (rc) goto out;
> > +        if (lock) {
> > +            rc = libxl__set_domain_configuration(gc, domid, &d_config);
> > +            if (rc) goto out;
> > +        }
> >  
> >          libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, 
> > back));
> >  
> > @@ -184,8 +194,10 @@ static int libxl__device_pci_add_xenstore(libxl__gc 
> > *gc, uint32_t domid, libxl_d
> >  out:
> >      libxl__xs_transaction_abort(gc, &t);
> >      if (lock) libxl__unlock_domain_userdata(lock);
> > -    libxl_device_pci_dispose(&pcidev_saved);
> > -    libxl_domain_config_dispose(&d_config);
> > +    if (!is_stubdomain) {
> > +        libxl_device_pci_dispose(&pcidev_saved);
> > +        libxl_domain_config_dispose(&d_config);
> > +    }
> >      return rc;
> >  }
> >  
> > -- 
> > git-series 0.9.1

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature

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