| Message ID | 1417802181-20834-6-git-send-email-tumanova@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
This patch looks basically good to me, I just got some cosmetic requests: On Fri, 5 Dec 2014 18:56:21 +0100 Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> wrote: ... > diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c > index 6fcf74d..fbd602d 100644 > --- a/hw/block/hd-geometry.c > +++ b/hw/block/hd-geometry.c > @@ -121,7 +121,17 @@ void hd_geometry_guess(BlockBackend *blk, > int *ptrans) > { > int cylinders, heads, secs, translation; > + hdGeometry geo; > > + /* Try to probe the backing device geometry, otherwise fallback > + to the old logic. (as of 12/2014 probing only succeeds on DASDs) */ > + if (blk_probe_geometry(blk, &geo) == 0) { > + *pcyls = geo.cylinders; > + *psecs = geo.sectors; > + *pheads = geo.heads; > + translation = BIOS_ATA_TRANSLATION_NONE; > + goto done; > + } > if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) { Can you get rid of the "goto" above by placing an "else" in front of this if-statement? > /* no LCHS guess: use a standard physical disk geometry */ > guess_chs_for_size(blk, pcyls, pheads, psecs); > @@ -142,7 +152,9 @@ void hd_geometry_guess(BlockBackend *blk, > /* disable any translation to be in sync with > the logical geometry */ > translation = BIOS_ATA_TRANSLATION_NONE; > + Unnecessary empty line, please remove this hunk. > } > +done: > if (ptrans) { > *ptrans = translation; > } Thomas
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: > geometry: hd_geometry_guess function autodetects the drive geometry. > This patch adds a block backend call, that probes the backing device > geometry. If the inner driver method is implemented and succeeds > (currently only for DASDs), the blkconf_geometry will pass-through > the backing device geometry. Otherwise will fallback to old logic. > > blocksize: This patch initializes blocksize properties to 0. > In order to set the properly a blkconf_blocksizes was introduced. > If user didn't set physical or logical blocksize, it will > retrieve it's value from a driver (which will return a default 512 for > any backend but DASD). > > The blkconf_blocksizes call was added to all users of BlkConf. > > Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> > --- > hw/block/block.c | 18 ++++++++++++++++++ > hw/block/hd-geometry.c | 12 ++++++++++++ > hw/block/nvme.c | 1 + > hw/block/virtio-blk.c | 1 + > hw/core/qdev-properties.c | 3 ++- > hw/ide/qdev.c | 1 + > hw/scsi/scsi-disk.c | 1 + > hw/usb/dev-storage.c | 1 + > include/hw/block/block.h | 5 +++-- > 9 files changed, 40 insertions(+), 3 deletions(-) > > diff --git a/hw/block/block.c b/hw/block/block.c > index a625773..9c07516 100644 > --- a/hw/block/block.c > +++ b/hw/block/block.c > @@ -25,6 +25,24 @@ void blkconf_serial(BlockConf *conf, char **serial) > } > } > > +void blkconf_blocksizes(BlockConf *conf) > +{ > + BlockBackend *blk = conf->blk; > + BlockSizes blocksizes; > + > + if (conf->physical_block_size && conf->logical_block_size) { > + return; > + } This conditional isn't necessary for correctness. Feel free to drop it. > + blk_probe_blocksizes(blk, &blocksizes); > + > + if (!conf->physical_block_size) { > + conf->physical_block_size = blocksizes.phys; > + } > + if (!conf->logical_block_size) { > + conf->logical_block_size = blocksizes.log; > + } This works because you change the default block size to 0 (second to last hunk). > +} > + > void blkconf_geometry(BlockConf *conf, int *ptrans, > unsigned cyls_max, unsigned heads_max, unsigned secs_max, > Error **errp) > diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c > index 6fcf74d..fbd602d 100644 > --- a/hw/block/hd-geometry.c > +++ b/hw/block/hd-geometry.c > @@ -121,7 +121,17 @@ void hd_geometry_guess(BlockBackend *blk, > int *ptrans) > { > int cylinders, heads, secs, translation; > + hdGeometry geo; > > + /* Try to probe the backing device geometry, otherwise fallback > + to the old logic. (as of 12/2014 probing only succeeds on DASDs) */ > + if (blk_probe_geometry(blk, &geo) == 0) { > + *pcyls = geo.cylinders; > + *psecs = geo.sectors; > + *pheads = geo.heads; > + translation = BIOS_ATA_TRANSLATION_NONE; > + goto done; > + } "else if" instead of goto, please. > if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) { > /* no LCHS guess: use a standard physical disk geometry */ > guess_chs_for_size(blk, pcyls, pheads, psecs); > @@ -142,7 +152,9 @@ void hd_geometry_guess(BlockBackend *blk, > /* disable any translation to be in sync with > the logical geometry */ > translation = BIOS_ATA_TRANSLATION_NONE; > + > } Humor me: put the empty line behind the brace instead of before. > +done: > if (ptrans) { > *ptrans = translation; > } Much easier to gauge than v2. Geometry change is a compatibility problem. You change it only when blk_probe_geometry() succeeds. It succeeds only for DASD. Mission accomplished. Recommend to add a hint to the function contract of the bdrv_probe_geometry() callback in block_int.h: /** * Try to get @bs's geometry (cyls, heads, sectos) * On success, store them in @geo and return 0. * On failure return -errno. * Only drivers that want to override guest geometry implement this * callback; see hd_geometry_guess(). */ int (*bdrv_probe_geometry)(BlockDriverState *bs, hdGeometry *geo); > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 1327658..244e382 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -763,6 +763,7 @@ static int nvme_init(PCIDevice *pci_dev) > if (!n->serial) { > return -1; > } > + blkconf_blocksizes(&n->conf); > > pci_conf = pci_dev->config; > pci_conf[PCI_INTERRUPT_PIN] = 1; > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index b19b102..6f01565 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -745,6 +745,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) > error_propagate(errp, err); > return; > } > + blkconf_blocksizes(&conf->conf); > > virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, > sizeof(struct virtio_blk_config)); > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 2e47f70..ba81709 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -580,7 +580,8 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque, > error_propagate(errp, local_err); > return; > } > - if (value < min || value > max) { > + /* value of 0 means "unset" */ > + if (value && (value < min || value > max)) { > error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, > dev->id?:"", name, (int64_t)value, min, max); > return; > diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c > index b4f096e..e71ff7f 100644 > --- a/hw/ide/qdev.c > +++ b/hw/ide/qdev.c > @@ -164,6 +164,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) > } > > blkconf_serial(&dev->conf, &dev->serial); > + blkconf_blocksizes(&dev->conf); > if (kind != IDE_CD) { > blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err); > if (err) { > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index 2f75d7d..5288129 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -2230,6 +2230,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp) > } > > blkconf_serial(&s->qdev.conf, &s->serial); > + blkconf_blocksizes(&s->qdev.conf); > if (dev->type == TYPE_DISK) { > blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err); > if (err) { > diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c > index 4539733..cc02dfd 100644 > --- a/hw/usb/dev-storage.c > +++ b/hw/usb/dev-storage.c > @@ -611,6 +611,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) > } > > blkconf_serial(&s->conf, &dev->serial); > + blkconf_blocksizes(&s->conf); > > /* > * Hack alert: this pretends to be a block device, but it's really > diff --git a/include/hw/block/block.h b/include/hw/block/block.h > index 0d0ce9a..3e502a8 100644 > --- a/include/hw/block/block.h > +++ b/include/hw/block/block.h > @@ -44,9 +44,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) > #define DEFINE_BLOCK_PROPERTIES(_state, _conf) \ > DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \ > DEFINE_PROP_BLOCKSIZE("logical_block_size", _state, \ > - _conf.logical_block_size, 512), \ > + _conf.logical_block_size, 0), \ > DEFINE_PROP_BLOCKSIZE("physical_block_size", _state, \ > - _conf.physical_block_size, 512), \ > + _conf.physical_block_size, 0), \ > DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0), \ > DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0), \ > DEFINE_PROP_UINT32("discard_granularity", _state, \ > @@ -63,6 +63,7 @@ void blkconf_serial(BlockConf *conf, char **serial); > void blkconf_geometry(BlockConf *conf, int *trans, > unsigned cyls_max, unsigned heads_max, unsigned secs_max, > Error **errp); > +void blkconf_blocksizes(BlockConf *conf); > > /* Hard disk geometry */
On 12/15/2014 04:50 PM, Markus Armbruster wrote: > Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: > >> geometry: hd_geometry_guess function autodetects the drive geometry. >> This patch adds a block backend call, that probes the backing device >> geometry. If the inner driver method is implemented and succeeds >> (currently only for DASDs), the blkconf_geometry will pass-through >> the backing device geometry. Otherwise will fallback to old logic. >> >> blocksize: This patch initializes blocksize properties to 0. >> In order to set the properly a blkconf_blocksizes was introduced. >> If user didn't set physical or logical blocksize, it will >> retrieve it's value from a driver (which will return a default 512 for >> any backend but DASD). >> >> The blkconf_blocksizes call was added to all users of BlkConf. >> >> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> >> --- >> hw/block/block.c | 18 ++++++++++++++++++ >> hw/block/hd-geometry.c | 12 ++++++++++++ >> hw/block/nvme.c | 1 + >> hw/block/virtio-blk.c | 1 + >> hw/core/qdev-properties.c | 3 ++- >> hw/ide/qdev.c | 1 + >> hw/scsi/scsi-disk.c | 1 + >> hw/usb/dev-storage.c | 1 + >> include/hw/block/block.h | 5 +++-- >> 9 files changed, 40 insertions(+), 3 deletions(-) >> >> diff --git a/hw/block/block.c b/hw/block/block.c >> index a625773..9c07516 100644 >> --- a/hw/block/block.c >> +++ b/hw/block/block.c >> @@ -25,6 +25,24 @@ void blkconf_serial(BlockConf *conf, char **serial) >> } >> } >> >> +void blkconf_blocksizes(BlockConf *conf) >> +{ >> + BlockBackend *blk = conf->blk; >> + BlockSizes blocksizes; >> + >> + if (conf->physical_block_size && conf->logical_block_size) { >> + return; >> + } > > This conditional isn't necessary for correctness. Feel free to drop it. > But this allows to avoid the ioctl call when user has specified both values. Remove anyway? >> + blk_probe_blocksizes(blk, &blocksizes); >> + >> + if (!conf->physical_block_size) { >> + conf->physical_block_size = blocksizes.phys; >> + } >> + if (!conf->logical_block_size) { >> + conf->logical_block_size = blocksizes.log; >> + } > I'll add a comment to make it apparent. > This works because you change the default block size to 0 (second to > last hunk). > >> +} >> + >> void blkconf_geometry(BlockConf *conf, int *ptrans, >> unsigned cyls_max, unsigned heads_max, unsigned secs_max, >> Error **errp) >> diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c >> index 6fcf74d..fbd602d 100644 >> --- a/hw/block/hd-geometry.c >> +++ b/hw/block/hd-geometry.c >> @@ -121,7 +121,17 @@ void hd_geometry_guess(BlockBackend *blk, >> int *ptrans) >> { >> int cylinders, heads, secs, translation; >> + hdGeometry geo; >> >> + /* Try to probe the backing device geometry, otherwise fallback >> + to the old logic. (as of 12/2014 probing only succeeds on DASDs) */ >> + if (blk_probe_geometry(blk, &geo) == 0) { >> + *pcyls = geo.cylinders; >> + *psecs = geo.sectors; >> + *pheads = geo.heads; >> + translation = BIOS_ATA_TRANSLATION_NONE; >> + goto done; >> + } > > "else if" instead of goto, please. > >> if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) { >> /* no LCHS guess: use a standard physical disk geometry */ >> guess_chs_for_size(blk, pcyls, pheads, psecs); >> @@ -142,7 +152,9 @@ void hd_geometry_guess(BlockBackend *blk, >> /* disable any translation to be in sync with >> the logical geometry */ >> translation = BIOS_ATA_TRANSLATION_NONE; >> + >> } > > Humor me: put the empty line behind the brace instead of before. > >> +done: >> if (ptrans) { >> *ptrans = translation; >> } > > Much easier to gauge than v2. Geometry change is a compatibility > problem. You change it only when blk_probe_geometry() succeeds. It > succeeds only for DASD. Mission accomplished. > > Recommend to add a hint to the function contract of the > bdrv_probe_geometry() callback in block_int.h: > > /** > * Try to get @bs's geometry (cyls, heads, sectos) > * On success, store them in @geo and return 0. > * On failure return -errno. > * Only drivers that want to override guest geometry implement this > * callback; see hd_geometry_guess(). > */ > int (*bdrv_probe_geometry)(BlockDriverState *bs, hdGeometry *geo); > >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c >> index 1327658..244e382 100644 >> --- a/hw/block/nvme.c >> +++ b/hw/block/nvme.c >> @@ -763,6 +763,7 @@ static int nvme_init(PCIDevice *pci_dev) >> if (!n->serial) { >> return -1; >> } >> + blkconf_blocksizes(&n->conf); >> >> pci_conf = pci_dev->config; >> pci_conf[PCI_INTERRUPT_PIN] = 1; >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index b19b102..6f01565 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -745,6 +745,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) >> error_propagate(errp, err); >> return; >> } >> + blkconf_blocksizes(&conf->conf); >> >> virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, >> sizeof(struct virtio_blk_config)); >> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >> index 2e47f70..ba81709 100644 >> --- a/hw/core/qdev-properties.c >> +++ b/hw/core/qdev-properties.c >> @@ -580,7 +580,8 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque, >> error_propagate(errp, local_err); >> return; >> } >> - if (value < min || value > max) { >> + /* value of 0 means "unset" */ >> + if (value && (value < min || value > max)) { >> error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, >> dev->id?:"", name, (int64_t)value, min, max); >> return; >> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c >> index b4f096e..e71ff7f 100644 >> --- a/hw/ide/qdev.c >> +++ b/hw/ide/qdev.c >> @@ -164,6 +164,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) >> } >> >> blkconf_serial(&dev->conf, &dev->serial); >> + blkconf_blocksizes(&dev->conf); >> if (kind != IDE_CD) { >> blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err); >> if (err) { >> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c >> index 2f75d7d..5288129 100644 >> --- a/hw/scsi/scsi-disk.c >> +++ b/hw/scsi/scsi-disk.c >> @@ -2230,6 +2230,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp) >> } >> >> blkconf_serial(&s->qdev.conf, &s->serial); >> + blkconf_blocksizes(&s->qdev.conf); >> if (dev->type == TYPE_DISK) { >> blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err); >> if (err) { >> diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c >> index 4539733..cc02dfd 100644 >> --- a/hw/usb/dev-storage.c >> +++ b/hw/usb/dev-storage.c >> @@ -611,6 +611,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) >> } >> >> blkconf_serial(&s->conf, &dev->serial); >> + blkconf_blocksizes(&s->conf); >> >> /* >> * Hack alert: this pretends to be a block device, but it's really >> diff --git a/include/hw/block/block.h b/include/hw/block/block.h >> index 0d0ce9a..3e502a8 100644 >> --- a/include/hw/block/block.h >> +++ b/include/hw/block/block.h >> @@ -44,9 +44,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) >> #define DEFINE_BLOCK_PROPERTIES(_state, _conf) \ >> DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \ >> DEFINE_PROP_BLOCKSIZE("logical_block_size", _state, \ >> - _conf.logical_block_size, 512), \ >> + _conf.logical_block_size, 0), \ >> DEFINE_PROP_BLOCKSIZE("physical_block_size", _state, \ >> - _conf.physical_block_size, 512), \ >> + _conf.physical_block_size, 0), \ >> DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0), \ >> DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0), \ >> DEFINE_PROP_UINT32("discard_granularity", _state, \ >> @@ -63,6 +63,7 @@ void blkconf_serial(BlockConf *conf, char **serial); >> void blkconf_geometry(BlockConf *conf, int *trans, >> unsigned cyls_max, unsigned heads_max, unsigned secs_max, >> Error **errp); >> +void blkconf_blocksizes(BlockConf *conf); >> >> /* Hard disk geometry */ >
Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: > On 12/15/2014 04:50 PM, Markus Armbruster wrote: >> Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> writes: >> >>> geometry: hd_geometry_guess function autodetects the drive geometry. >>> This patch adds a block backend call, that probes the backing device >>> geometry. If the inner driver method is implemented and succeeds >>> (currently only for DASDs), the blkconf_geometry will pass-through >>> the backing device geometry. Otherwise will fallback to old logic. >>> >>> blocksize: This patch initializes blocksize properties to 0. >>> In order to set the properly a blkconf_blocksizes was introduced. >>> If user didn't set physical or logical blocksize, it will >>> retrieve it's value from a driver (which will return a default 512 for >>> any backend but DASD). >>> >>> The blkconf_blocksizes call was added to all users of BlkConf. >>> >>> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> >>> --- >>> hw/block/block.c | 18 ++++++++++++++++++ >>> hw/block/hd-geometry.c | 12 ++++++++++++ >>> hw/block/nvme.c | 1 + >>> hw/block/virtio-blk.c | 1 + >>> hw/core/qdev-properties.c | 3 ++- >>> hw/ide/qdev.c | 1 + >>> hw/scsi/scsi-disk.c | 1 + >>> hw/usb/dev-storage.c | 1 + >>> include/hw/block/block.h | 5 +++-- >>> 9 files changed, 40 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/block/block.c b/hw/block/block.c >>> index a625773..9c07516 100644 >>> --- a/hw/block/block.c >>> +++ b/hw/block/block.c >>> @@ -25,6 +25,24 @@ void blkconf_serial(BlockConf *conf, char **serial) >>> } >>> } >>> >>> +void blkconf_blocksizes(BlockConf *conf) >>> +{ >>> + BlockBackend *blk = conf->blk; >>> + BlockSizes blocksizes; >>> + >>> + if (conf->physical_block_size && conf->logical_block_size) { >>> + return; >>> + } >> >> This conditional isn't necessary for correctness. Feel free to drop it. >> > > But this allows to avoid the ioctl call when user has specified both > values. Remove anyway? It's device model setup, i.e. not a fast path. I'd favor simplicity and compactness over speed there. >>> + blk_probe_blocksizes(blk, &blocksizes); >>> + >>> + if (!conf->physical_block_size) { >>> + conf->physical_block_size = blocksizes.phys; >>> + } >>> + if (!conf->logical_block_size) { >>> + conf->logical_block_size = blocksizes.log; >>> + } >> > > I'll add a comment to make it apparent. > >> This works because you change the default block size to 0 (second to >> last hunk). >> >>> +} >>> + >>> void blkconf_geometry(BlockConf *conf, int *ptrans, >>> unsigned cyls_max, unsigned heads_max, unsigned secs_max, >>> Error **errp) [...]
diff --git a/hw/block/block.c b/hw/block/block.c index a625773..9c07516 100644 --- a/hw/block/block.c +++ b/hw/block/block.c @@ -25,6 +25,24 @@ void blkconf_serial(BlockConf *conf, char **serial) } } +void blkconf_blocksizes(BlockConf *conf) +{ + BlockBackend *blk = conf->blk; + BlockSizes blocksizes; + + if (conf->physical_block_size && conf->logical_block_size) { + return; + } + blk_probe_blocksizes(blk, &blocksizes); + + if (!conf->physical_block_size) { + conf->physical_block_size = blocksizes.phys; + } + if (!conf->logical_block_size) { + conf->logical_block_size = blocksizes.log; + } +} + void blkconf_geometry(BlockConf *conf, int *ptrans, unsigned cyls_max, unsigned heads_max, unsigned secs_max, Error **errp) diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c index 6fcf74d..fbd602d 100644 --- a/hw/block/hd-geometry.c +++ b/hw/block/hd-geometry.c @@ -121,7 +121,17 @@ void hd_geometry_guess(BlockBackend *blk, int *ptrans) { int cylinders, heads, secs, translation; + hdGeometry geo; + /* Try to probe the backing device geometry, otherwise fallback + to the old logic. (as of 12/2014 probing only succeeds on DASDs) */ + if (blk_probe_geometry(blk, &geo) == 0) { + *pcyls = geo.cylinders; + *psecs = geo.sectors; + *pheads = geo.heads; + translation = BIOS_ATA_TRANSLATION_NONE; + goto done; + } if (guess_disk_lchs(blk, &cylinders, &heads, &secs) < 0) { /* no LCHS guess: use a standard physical disk geometry */ guess_chs_for_size(blk, pcyls, pheads, psecs); @@ -142,7 +152,9 @@ void hd_geometry_guess(BlockBackend *blk, /* disable any translation to be in sync with the logical geometry */ translation = BIOS_ATA_TRANSLATION_NONE; + } +done: if (ptrans) { *ptrans = translation; } diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 1327658..244e382 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -763,6 +763,7 @@ static int nvme_init(PCIDevice *pci_dev) if (!n->serial) { return -1; } + blkconf_blocksizes(&n->conf); pci_conf = pci_dev->config; pci_conf[PCI_INTERRUPT_PIN] = 1; diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index b19b102..6f01565 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -745,6 +745,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) error_propagate(errp, err); return; } + blkconf_blocksizes(&conf->conf); virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, sizeof(struct virtio_blk_config)); diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 2e47f70..ba81709 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -580,7 +580,8 @@ static void set_blocksize(Object *obj, Visitor *v, void *opaque, error_propagate(errp, local_err); return; } - if (value < min || value > max) { + /* value of 0 means "unset" */ + if (value && (value < min || value > max)) { error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, dev->id?:"", name, (int64_t)value, min, max); return; diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index b4f096e..e71ff7f 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -164,6 +164,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind) } blkconf_serial(&dev->conf, &dev->serial); + blkconf_blocksizes(&dev->conf); if (kind != IDE_CD) { blkconf_geometry(&dev->conf, &dev->chs_trans, 65536, 16, 255, &err); if (err) { diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 2f75d7d..5288129 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -2230,6 +2230,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp) } blkconf_serial(&s->qdev.conf, &s->serial); + blkconf_blocksizes(&s->qdev.conf); if (dev->type == TYPE_DISK) { blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err); if (err) { diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index 4539733..cc02dfd 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -611,6 +611,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) } blkconf_serial(&s->conf, &dev->serial); + blkconf_blocksizes(&s->conf); /* * Hack alert: this pretends to be a block device, but it's really diff --git a/include/hw/block/block.h b/include/hw/block/block.h index 0d0ce9a..3e502a8 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -44,9 +44,9 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) #define DEFINE_BLOCK_PROPERTIES(_state, _conf) \ DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \ DEFINE_PROP_BLOCKSIZE("logical_block_size", _state, \ - _conf.logical_block_size, 512), \ + _conf.logical_block_size, 0), \ DEFINE_PROP_BLOCKSIZE("physical_block_size", _state, \ - _conf.physical_block_size, 512), \ + _conf.physical_block_size, 0), \ DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0), \ DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0), \ DEFINE_PROP_UINT32("discard_granularity", _state, \ @@ -63,6 +63,7 @@ void blkconf_serial(BlockConf *conf, char **serial); void blkconf_geometry(BlockConf *conf, int *trans, unsigned cyls_max, unsigned heads_max, unsigned secs_max, Error **errp); +void blkconf_blocksizes(BlockConf *conf); /* Hard disk geometry */
geometry: hd_geometry_guess function autodetects the drive geometry. This patch adds a block backend call, that probes the backing device geometry. If the inner driver method is implemented and succeeds (currently only for DASDs), the blkconf_geometry will pass-through the backing device geometry. Otherwise will fallback to old logic. blocksize: This patch initializes blocksize properties to 0. In order to set the properly a blkconf_blocksizes was introduced. If user didn't set physical or logical blocksize, it will retrieve it's value from a driver (which will return a default 512 for any backend but DASD). The blkconf_blocksizes call was added to all users of BlkConf. Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com> --- hw/block/block.c | 18 ++++++++++++++++++ hw/block/hd-geometry.c | 12 ++++++++++++ hw/block/nvme.c | 1 + hw/block/virtio-blk.c | 1 + hw/core/qdev-properties.c | 3 ++- hw/ide/qdev.c | 1 + hw/scsi/scsi-disk.c | 1 + hw/usb/dev-storage.c | 1 + include/hw/block/block.h | 5 +++-- 9 files changed, 40 insertions(+), 3 deletions(-)