Re: [PATCH v2] blkif: reconcile protocol specification with in-use implementations

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Sep 10, 2024 at 04:01:50PM +0200, Jürgen Groß wrote:
> On 10.09.24 15:59, Roger Pau Monné wrote:
> > On Tue, Sep 10, 2024 at 03:46:00PM +0200, Jürgen Groß wrote:
> > > On 10.09.24 13:46, Roger Pau Monne wrote:
> > > > diff --git a/xen/include/public/io/blkif.h 
> > > > b/xen/include/public/io/blkif.h
> > > > index 22f1eef0c0ca..da893eb379db 100644
> > > > --- a/xen/include/public/io/blkif.h
> > > > +++ b/xen/include/public/io/blkif.h
> > > > @@ -237,12 +237,16 @@
> > > >     * sector-size
> > > >     *      Values:         <uint32_t>
> > > >     *
> > > > - *      The logical block size, in bytes, of the underlying storage. 
> > > > This
> > > > - *      must be a power of two with a minimum value of 512.
> > > > + *      The logical block size, in bytes, of the underlying storage. 
> > > > This must
> > > > + *      be a power of two with a minimum value of 512.  The sector 
> > > > size should
> > > > + *      only be used for request segment length and alignment.
> > > >     *
> > > > - *      NOTE: Because of implementation bugs in some frontends this 
> > > > must be
> > > > - *            set to 512, unless the frontend advertizes a non-zero 
> > > > value
> > > > - *            in its "feature-large-sector-size" xenbus node. (See 
> > > > below).
> > > > + *      When exposing a device that uses 4096 logical sector sizes, 
> > > > the only
> > >
> > > s/uses 4096 logical sector sizes/uses a logical sector size of 4096/
> >
> > Yes, that's better.
> >
> > > > + *      difference xenstore wise will be that 'sector-size' (and 
> > > > possibly
> > > > + *      'physical-sector-size' if supported by the backend) will be 
> > > > 4096, but
> > > > + *      the 'sectors' node will still be calculated using 512 byte 
> > > > units.  The
> > > > + *      sector base units in the ring requests fields will all be 512 
> > > > byte
> > > > + *      based despite the logical sector size exposed in 'sector-size'.
> > > >     *
> > > >     * physical-sector-size
> > > >     *      Values:         <uint32_t>
> > > > @@ -254,8 +258,8 @@
> > > >     * sectors
> > > >     *      Values:         <uint64_t>
> > > >     *
> > > > - *      The size of the backend device, expressed in units of 
> > > > "sector-size".
> > > > - *      The product of "sector-size" and "sectors" must also be an 
> > > > integer
> > > > + *      The size of the backend device, expressed in units of 512b.
> > > > + *      The product of "sector-size" * 512 must also be an integer
> > >
> > > DYM: The product of "sectors" * 512 must also be an integer ... ?
> >
> > Indeed.
>
> With those 2 changes applied you can add my:
>
> Reviewed-by: Juergen Gross <jgross@xxxxxxxx>

Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Cheers,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech