Re: [Xen-devel] [PATCH RFC 7/8] xen-blkback: frontend feature control

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Feb 07, 2018 at 02:16:14PM +0000, Joao Martins wrote:
> On 02/07/2018 12:08 PM, Roger Pau Monné wrote:
> > On Thu, Nov 02, 2017 at 06:06:15PM +0000, Joao Martins wrote:
> >> +static void xen_blkbk_probe_features(struct xenbus_device *dev,
> >> +                               struct backend_info *be)
> >> +{
> >> +  struct backend_features *ft = &be->features;
> >> +  struct xen_vbd *vbd = &be->blkif->vbd;
> >> +
> > 
> > #define READ_FEAT(path, feat, default) \
> >     xenbus_read_unsigned(path, "require/" node, default)
> > 
> >> +  vbd->max_indirect_segs = xenbus_read_feature(dev->nodename,
> >> +                                          "feature-max-indirect-segments",
> >> +                                          MAX_INDIRECT_SEGMENTS);
> >> +
> >> +  ft->max_queues = xenbus_read_feature(dev->nodename,
> >> +                                       "multi-queue-max-queues",
> >> +                                       xenblk_max_queues);
> >> +
> >> +  ft->max_ring_order = xenbus_read_feature(dev->nodename,
> >> +                                           "max-ring-page-order",
> >> +                                           xen_blkif_max_ring_order);
> >> +
> >> +  ft->pers_grants = xenbus_read_feature(dev->nodename,
> >> +                                        "feature-persistent",
> >> +                                        1);
> > #undef READ_FEAT
> > 
> > Aren't you missing some check here or elsewhere to make sure the
> > proposed values don't exceed the maximum ones supported by blback?
> > 
> > I would expect something like:
> > 
> > vbd->max_indirect_segs = min(vbd->max_indirect_segs, MAX_INDIRECT_SEGMENTS);
> > 
> > And the same with max_queues and max_ring_oder.
> > 
> 
> This was deliberate to some extent. How do we define the value of max_queues?
> Relying on the module parameters seems wrong as those *also* represent default
> values for all guests. So I wouldn't cap to xen_blkbk_max_queues because it
> would be left out to toolstack to pick. indirect_segs and max_ring_order there
> are actual maximum values defined in macros so those definitely make sense to
> check/validate.

Yes, the maximum number of indirect segments is ATM a compile time
value, so exposing something bigger to the guest will only end up in
failed requests.

Thanks, Roger.

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