Re: [Xen-devel] [v3][PATCH 12/16] tools/libxl: passes rdm reservation policy

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2015/6/13 0:17, Wei Liu wrote:
> On Thu, Jun 11, 2015 at 09:15:21AM +0800, Tiejun Chen wrote:
> > This patch passes our rdm reservation policy inside libxl
> > when we assign a device or attach a device.
> >
> > Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
> > ---
> >   docs/man/xl.pod.1         |  7 ++++++-
> >   tools/libxl/libxl_pci.c   | 10 +++++++++-
> >   tools/libxl/xl_cmdimpl.c  | 23 +++++++++++++++++++----
> >   tools/libxl/xl_cmdtable.c |  2 +-
> >   4 files changed, 35 insertions(+), 7 deletions(-)
> >
> > diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> > index 4eb929d..c5c4809 100644
> > --- a/docs/man/xl.pod.1
> > +++ b/docs/man/xl.pod.1
> > @@ -1368,10 +1368,15 @@ it will also attempt to re-bind the device to its 
> > original driver, making it
> >   usable by Domain 0 again.  If the device is not bound to pciback, it will
> >   return success.
> >
> > -=item B<pci-attach> I<domain-id> I<BDF>
> > +=item B<pci-attach> I<domain-id> I<BDF> [I<rdm>]
> >
> >   Hot-plug a new pass-through pci device to the specified domain.
> >   B<BDF> is the PCI Bus/Device/Function of the physical device to pass-through.
> > +B<rdm policy> is about how to handle conflict between reserving reserved device
> > +memory and guest address space. "strict" means an unsolved conflict leads to
> > +immediate VM crash, while "relaxed" allows VM moving forward with a warning
> > +message thrown out. Here "strict" is default.
> > +
> >
> >   =item B<pci-detach> [I<-f>] I<domain-id> I<BDF>
> >
> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index a00d799..d2e8911 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -894,7 +894,7 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, 
> > libxl_device_pci *pcidev, i
> >       FILE *f;
> >       unsigned long long start, end, flags, size;
> >       int irq, i, rc, hvm = 0;
> > -    uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
> > +    uint32_t flag;
> >
> >       if (type == LIBXL_DOMAIN_TYPE_INVALID)
> >           return ERROR_FAIL;
> > @@ -988,6 +988,14 @@ static int do_pci_add(libxl__gc *gc, uint32_t domid, 
> > libxl_device_pci *pcidev, i
> >
> >   out:
> >       if (!libxl_is_stubdom(ctx, domid, NULL)) {
> > +        if (pcidev->rdm_reserve == LIBXL_RDM_RESERVE_FLAG_RELAXED) {
> > +            flag = XEN_DOMCTL_DEV_RDM_RELAXED;
> > +        } else if (pcidev->rdm_reserve == LIBXL_RDM_RESERVE_FLAG_STRICT) {
> > +            flag = XEN_DOMCTL_DEV_RDM_STRICT;
> > +        } else {
> > +            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unkwon rdm check flag.");
> Typo "unkwon" and use LOG(ERROR,...).
Will fix that typo, s/unkwon/unknown, but are you sure we should use 
LOG() here? Because this function always uses LIBXL__LOG_ERRNO(),
> > +            return ERROR_FAIL;
> > +        }
> >           rc = xc_assign_device(ctx->xch, domid, pcidev_encode_bdf(pcidev), 
> > flag);
> >           if (rc < 0 && (hvm || errno != ENOSYS)) {
> >               LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xc_assign_device 
> > failed");
like here.

> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index aedbd4b..4364ba4 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -3359,7 +3359,8 @@ int main_pcidetach(int argc, char **argv)
> >       pcidetach(domid, bdf, force);
> >       return 0;
> >   }
> > -static void pciattach(uint32_t domid, const char *bdf, const char *vs)
> > +static void pciattach(uint32_t domid, const char *bdf, const char *vs,
> > +                      uint32_t flag)
> >   {
> >       libxl_device_pci pcidev;
> >       XLU_Config *config;
> > @@ -3369,6 +3370,7 @@ static void pciattach(uint32_t domid, const char *bdf, 
> > const char *vs)
> >       config = xlu_cfg_init(stderr, "command line");
> >       if (!config) { perror("xlu_cfg_inig"); exit(-1); }
> >
> > +    pcidev.rdm_reserve = flag;
> >       if (xlu_pci_parse_bdf(config, &pcidev, bdf)) {
> >           fprintf(stderr, "pci-attach: malformed BDF specification \"%s\"\n", 
> > bdf);
> >           exit(2);
> > @@ -3381,9 +3383,9 @@ static void pciattach(uint32_t domid, const char *bdf, 
> > const char *vs)
> >
> >   int main_pciattach(int argc, char **argv)
> >   {
> > -    uint32_t domid;
> > +    uint32_t domid, flag;
> >       int opt;
> > -    const char *bdf = NULL, *vs = NULL;
> > +    const char *bdf = NULL, *vs = NULL, *rdm_policy = NULL;
> >
> >       SWITCH_FOREACH_OPT(opt, "", NULL, "pci-attach", 2) {
> >           /* No options */
> > @@ -3395,7 +3397,20 @@ int main_pciattach(int argc, char **argv)
> >       if (optind + 1 < argc)
> >           vs = argv[optind + 2];
> >
> > -    pciattach(domid, bdf, vs);
> > +    if (optind + 2 < argc) {
> > +        rdm_policy = argv[optind + 3];
> > +    }
> > +    if (!strcmp(rdm_policy, "strict")) {
> > +        flag = LIBXL_RDM_RESERVE_FLAG_STRICT;
> > +    } else if (!strcmp(rdm_policy, "relaxed")) {
> > +        flag = LIBXL_RDM_RESERVE_FLAG_RELAXED;
> > +    } else {
> > +        fprintf(stderr, "%s is an invalid rdm policy: 'strict'|'relaxed'\n",
> > +                rdm_policy);
> > +        exit(2);
> > +    }
> > +
> > +    pciattach(domid, bdf, vs, flag);
> >       return 0;
> >   }
> >
> > diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> > index 7f4759b..552fbec 100644
> > --- a/tools/libxl/xl_cmdtable.c
> > +++ b/tools/libxl/xl_cmdtable.c
> > @@ -88,7 +88,7 @@ struct cmd_spec cmd_table[] = {
> >       { "pci-attach",
> >         &main_pciattach, 0, 1,
> >         "Insert a new pass-through pci device",
> > -      "<Domain> <BDF> [Virtual Slot]",
> > +      "<Domain> <BDF> [Virtual Slot] <policy to reserve 
> > rdm['strice'|'relaxed']>",
> Should use "[]" to indicate it's optional.
What about this?

"<Domain> <BDF> [Virtual Slot] [policy to reserve rdm<'strice'|'relaxed'>]",

Thanks
Tiejun

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