Re: [Qemu-block] [Qemu-devel] [PATCH v6 1/4] block: Add zoned device model property

[Dmitry Fomichev]

On Fri, 2019-09-06 at 23:10 +0200, Stefano Garzarella wrote:
> On Fri, Sep 06, 2019 at 04:17:12PM +0000, Dmitry Fomichev wrote:
> > On Fri, 2019-09-06 at 10:11 +0200, Stefano Garzarella wrote:
> > > On Wed, Sep 04, 2019 at 05:00:57PM -0400, Dmitry Fomichev wrote:
> > > > This commit adds Zoned Device Model (as defined in T10 ZBC and
> > > > T13 ZAC standards) as a block driver property, along with some
> > > > useful access functions.
> > > > 
> > > > A new backend driver permission, BLK_PERM_SUPPORT_HM_ZONED, is also
> > > > introduced. Only the drivers having this permission will be allowed
> > > > to open host managed zoned block devices.
> > > > 
> > > > No code is added yet to initialize or check the value of this new
> > > > property, therefore this commit doesn't change any functionality.
> > > > 
> > > > Signed-off-by: Dmitry Fomichev <address@hidden>
> > > > ---
> > > >  block.c                   | 15 +++++++++++++++
> > > >  include/block/block.h     | 19 ++++++++++++++++++-
> > > >  include/block/block_int.h |  3 +++
> > > >  3 files changed, 36 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/block.c b/block.c
> > > > index 874a29a983..69f565e1e9 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -4679,6 +4679,21 @@ void bdrv_get_geometry(BlockDriverState *bs, 
> > > > uint64_t *nb_sectors_ptr)
> > > >      *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
> > > >  }
> > > >  
> > > > +BdrvZonedModel bdrv_get_zoned_model(BlockDriverState *bs)
> > > > +{
> > > > +    return bs->bl.zoned_model;
> > > > +}
> > > > +
> > > > +bool bdrv_is_hm_zoned(BlockDriverState *bs)
> > > > +{
> > > > +    /*
> > > > +     * Host Aware zone devices are supposed to be able to work
> > > > +     * just like regular block devices. Thus, we only consider
> > > > +     * Host Managed devices to be zoned here.
> > > > +     */
> > > > +    return bdrv_get_zoned_model(bs) == BDRV_ZONED_MODEL_HM;
> > > > +}
> > > > +
> > > >  bool bdrv_is_sg(BlockDriverState *bs)
> > > >  {
> > > >      return bs->sg;
> > > > diff --git a/include/block/block.h b/include/block/block.h
> > > > index 124ad40809..28d065ed80 100644
> > > > --- a/include/block/block.h
> > > > +++ b/include/block/block.h
> > > > @@ -271,18 +271,33 @@ enum {
> > > >       */
> > > >      BLK_PERM_GRAPH_MOD          = 0x10,
> > > >  
> > > > +    /**
> > > > +     * This permission is required to open host-managed zoned block 
> > > > devices.
> > > > +     */
> > > > +    BLK_PERM_SUPPORT_HM_ZONED   = 0x20,
> > > > +
> > > >      BLK_PERM_ALL                = 0x1f,
> > > 
> > > Should we update BLK_PERM_ALL to 0x3f?
> > > 
> > Stefano, good catch! Will update and resend...
> > 
> 
> Good!
> 
> Looking better, if we update it, maybe we should also change something in
> xdbg_graph_add_edge() since there is this line:
> 
>     QEMU_BUILD_BUG_ON(1UL << (ARRAY_SIZE(permissions) - 1) != BLK_PERM_ALL + 
> 1);
> 
> We should extend the permissions array or change this check.
> 
Yes, I've noticed that the mask change triggers this BUG_ON.
I am adding BLK_PERM_SUPPORT_HM_ZONED permission into "permissions" array and 
this
also requires adding the permission to qapi schema. I think that would be the 
right
thing to do rather than to modify this check.

Dmitry

> Thanks,
> Stefano