| Message ID | 1354631742-4693-7-git-send-email-fred.konrad@greensocs.com |
|---|---|
| State | New |
| Headers | show |
On 4 December 2012 14:35, <fred.konrad@greensocs.com> wrote: > From: KONRAD Frederic <fred.konrad@greensocs.com> > > Create virtio-blk which extends virtio-device, so it can be connected on > virtio-bus. > > Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> > --- > hw/virtio-blk.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++-------- > hw/virtio-blk.h | 4 ++ > 2 files changed, 150 insertions(+), 24 deletions(-) > > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index e25cc96..ee1ea8b 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -21,24 +21,42 @@ > #ifdef __linux__ > # include <scsi/sg.h> > #endif > +#include "virtio-bus.h" > > +/* Take this structure as our device structure. */ > typedef struct VirtIOBlock > { > + /* > + * Adding parent_obj breaks to_virtio_blk cast function, > + * and virtio_blk_init. > + */ > + DeviceState parent_obj; > + /* > + * We don't need that anymore, as we'll use QOM cast to get the > + * VirtIODevice. Just temporary keep it, for not breaking functionality. > + */ > VirtIODevice vdev; This doesn't make sense. After your previous patch, VirtIODevice is-a DeviceState, and VirtIOBlock already is-a VirtIODevice, so there's nothing to do here. Adding this parent_obj field here is just breaking things (it would make the VirtIOBlock into a direct child of DeviceState, which isn't what we want). > BlockDriverState *bs; > VirtQueue *vq; > void *rq; > QEMUBH *bh; > BlockConf *conf; > - VirtIOBlkConf *blk; > + /* > + * We can't use pointer with properties. > + */ > + VirtIOBlkConf blk; > unsigned short sector_mask; > DeviceState *qdev; > } VirtIOBlock; > > -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) > -{ > - return (VirtIOBlock *)vdev; > -} > +/* > + * Use the QOM cast, so we don't need that anymore. > + * > + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) > + * { > + * return (VirtIOBlock *)vdev; > + * } > + */ If we don't need it, just delete it. -- PMM
Am 05.12.2012 17:25, schrieb Peter Maydell: > On 4 December 2012 14:35, <fred.konrad@greensocs.com> wrote: >> From: KONRAD Frederic <fred.konrad@greensocs.com> >> >> Create virtio-blk which extends virtio-device, so it can be connected on >> virtio-bus. >> >> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >> --- >> hw/virtio-blk.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++-------- >> hw/virtio-blk.h | 4 ++ >> 2 files changed, 150 insertions(+), 24 deletions(-) >> >> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c >> index e25cc96..ee1ea8b 100644 >> --- a/hw/virtio-blk.c >> +++ b/hw/virtio-blk.c >> @@ -21,24 +21,42 @@ >> #ifdef __linux__ >> # include <scsi/sg.h> >> #endif >> +#include "virtio-bus.h" >> >> +/* Take this structure as our device structure. */ >> typedef struct VirtIOBlock >> { >> + /* >> + * Adding parent_obj breaks to_virtio_blk cast function, >> + * and virtio_blk_init. >> + */ >> + DeviceState parent_obj; >> + /* >> + * We don't need that anymore, as we'll use QOM cast to get the >> + * VirtIODevice. Just temporary keep it, for not breaking functionality. >> + */ >> VirtIODevice vdev; > > This doesn't make sense. After your previous patch, VirtIODevice > is-a DeviceState, and VirtIOBlock already is-a VirtIODevice, > so there's nothing to do here. Adding this parent_obj field > here is just breaking things (it would make the VirtIOBlock > into a direct child of DeviceState, which isn't what we want). > >> BlockDriverState *bs; >> VirtQueue *vq; >> void *rq; >> QEMUBH *bh; >> BlockConf *conf; >> - VirtIOBlkConf *blk; >> + /* >> + * We can't use pointer with properties. >> + */ >> + VirtIOBlkConf blk; >> unsigned short sector_mask; >> DeviceState *qdev; >> } VirtIOBlock; >> >> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >> -{ >> - return (VirtIOBlock *)vdev; >> -} >> +/* >> + * Use the QOM cast, so we don't need that anymore. >> + * >> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >> + * { >> + * return (VirtIOBlock *)vdev; >> + * } >> + */ > > If we don't need it, just delete it. Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by OBJECT_CHECK(), and replace all callers of to_virtio_blk() with VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you on. You can then rename vdev to parent_obj as a check that you caught all users. Regards, Andreas
On 05/12/2012 17:25, Peter Maydell wrote: > On 4 December 2012 14:35, <fred.konrad@greensocs.com> wrote: >> From: KONRAD Frederic <fred.konrad@greensocs.com> >> >> Create virtio-blk which extends virtio-device, so it can be connected on >> virtio-bus. >> >> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >> --- >> hw/virtio-blk.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++-------- >> hw/virtio-blk.h | 4 ++ >> 2 files changed, 150 insertions(+), 24 deletions(-) >> >> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c >> index e25cc96..ee1ea8b 100644 >> --- a/hw/virtio-blk.c >> +++ b/hw/virtio-blk.c >> @@ -21,24 +21,42 @@ >> #ifdef __linux__ >> # include <scsi/sg.h> >> #endif >> +#include "virtio-bus.h" >> >> +/* Take this structure as our device structure. */ >> typedef struct VirtIOBlock >> { >> + /* >> + * Adding parent_obj breaks to_virtio_blk cast function, >> + * and virtio_blk_init. >> + */ >> + DeviceState parent_obj; >> + /* >> + * We don't need that anymore, as we'll use QOM cast to get the >> + * VirtIODevice. Just temporary keep it, for not breaking functionality. >> + */ >> VirtIODevice vdev; > This doesn't make sense. After your previous patch, VirtIODevice > is-a DeviceState, and VirtIOBlock already is-a VirtIODevice, > so there's nothing to do here. Adding this parent_obj field > here is just breaking things (it would make the VirtIOBlock > into a direct child of DeviceState, which isn't what we want). > Ok, I think you're right, I'll check. >> BlockDriverState *bs; >> VirtQueue *vq; >> void *rq; >> QEMUBH *bh; >> BlockConf *conf; >> - VirtIOBlkConf *blk; >> + /* >> + * We can't use pointer with properties. >> + */ >> + VirtIOBlkConf blk; >> unsigned short sector_mask; >> DeviceState *qdev; >> } VirtIOBlock; >> >> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >> -{ >> - return (VirtIOBlock *)vdev; >> -} >> +/* >> + * Use the QOM cast, so we don't need that anymore. >> + * >> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >> + * { >> + * return (VirtIOBlock *)vdev; >> + * } >> + */ > If we don't need it, just delete it. > > -- PMM Yes, sure, I put it in comment to explain, why I deleted it. Thanks, Fred.
Am 06.12.2012 10:11, schrieb KONRAD Frédéric: > On 05/12/2012 17:25, Peter Maydell wrote: >> On 4 December 2012 14:35, <fred.konrad@greensocs.com> wrote: >>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>> -{ >>> - return (VirtIOBlock *)vdev; >>> -} >>> +/* >>> + * Use the QOM cast, so we don't need that anymore. >>> + * >>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>> + * { >>> + * return (VirtIOBlock *)vdev; >>> + * } >>> + */ >> If we don't need it, just delete it. >> >> -- PMM > Yes, sure, I put it in comment to explain, why I deleted it. Please don't comment out unneeded things. Instead, remove them completely and put the explanation into the commit message. That helps keep the patch small and avoids commented-out code bitrotting. Andreas
On 05/12/2012 18:22, Andreas Färber wrote: > Am 05.12.2012 17:25, schrieb Peter Maydell: >> On 4 December 2012 14:35, <fred.konrad@greensocs.com> wrote: >>> From: KONRAD Frederic <fred.konrad@greensocs.com> >>> >>> Create virtio-blk which extends virtio-device, so it can be connected on >>> virtio-bus. >>> >>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >>> --- >>> hw/virtio-blk.c | 170 ++++++++++++++++++++++++++++++++++++++++++++++++-------- >>> hw/virtio-blk.h | 4 ++ >>> 2 files changed, 150 insertions(+), 24 deletions(-) >>> >>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c >>> index e25cc96..ee1ea8b 100644 >>> --- a/hw/virtio-blk.c >>> +++ b/hw/virtio-blk.c >>> @@ -21,24 +21,42 @@ >>> #ifdef __linux__ >>> # include <scsi/sg.h> >>> #endif >>> +#include "virtio-bus.h" >>> >>> +/* Take this structure as our device structure. */ >>> typedef struct VirtIOBlock >>> { >>> + /* >>> + * Adding parent_obj breaks to_virtio_blk cast function, >>> + * and virtio_blk_init. >>> + */ >>> + DeviceState parent_obj; >>> + /* >>> + * We don't need that anymore, as we'll use QOM cast to get the >>> + * VirtIODevice. Just temporary keep it, for not breaking functionality. >>> + */ >>> VirtIODevice vdev; >> This doesn't make sense. After your previous patch, VirtIODevice >> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice, >> so there's nothing to do here. Adding this parent_obj field >> here is just breaking things (it would make the VirtIOBlock >> into a direct child of DeviceState, which isn't what we want). >> >>> BlockDriverState *bs; >>> VirtQueue *vq; >>> void *rq; >>> QEMUBH *bh; >>> BlockConf *conf; >>> - VirtIOBlkConf *blk; >>> + /* >>> + * We can't use pointer with properties. >>> + */ >>> + VirtIOBlkConf blk; >>> unsigned short sector_mask; >>> DeviceState *qdev; >>> } VirtIOBlock; >>> >>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>> -{ >>> - return (VirtIOBlock *)vdev; >>> -} >>> +/* >>> + * Use the QOM cast, so we don't need that anymore. >>> + * >>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>> + * { >>> + * return (VirtIOBlock *)vdev; >>> + * } >>> + */ >> If we don't need it, just delete it. > Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by > OBJECT_CHECK(), and replace all callers of to_virtio_blk() with > VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you on. Yes, that's why I comment to_virtio_blk(). Isn't what I made in this patch with : +#define TYPE_VIRTIO_BLK "virtio-blk" +#define VIRTIO_BLK(obj) \ + OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK) + and - VirtIOBlock *s = to_virtio_blk(vdev); + VirtIOBlock *s = VIRTIO_BLK(vdev); ? I agree with that, but, there is an issue : The refactored VirtIOBlk is a device and seems to work, but the device which use this VirtIOBlock (eg virtio-blk-pci) are just allocating a structure ( in virtio_common_init ). That's why this patch is breaking virtio-blk-pci. > > Regards, > Andreas > Thanks, Fred.
On 06/12/2012 10:18, Andreas Färber wrote: > Am 06.12.2012 10:11, schrieb KONRAD Frédéric: >> On 05/12/2012 17:25, Peter Maydell wrote: >>> On 4 December 2012 14:35, <fred.konrad@greensocs.com> wrote: >>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>>> -{ >>>> - return (VirtIOBlock *)vdev; >>>> -} >>>> +/* >>>> + * Use the QOM cast, so we don't need that anymore. >>>> + * >>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>>> + * { >>>> + * return (VirtIOBlock *)vdev; >>>> + * } >>>> + */ >>> If we don't need it, just delete it. >>> >>> -- PMM >> Yes, sure, I put it in comment to explain, why I deleted it. > Please don't comment out unneeded things. Instead, remove them > completely and put the explanation into the commit message. That helps > keep the patch small and avoids commented-out code bitrotting. > > Andreas > Ok, no problem.
Am 06.12.2012 10:21, schrieb KONRAD Frédéric: > On 05/12/2012 18:22, Andreas Färber wrote: >> Am 05.12.2012 17:25, schrieb Peter Maydell: >>> On 4 December 2012 14:35, <fred.konrad@greensocs.com> wrote: >>>> From: KONRAD Frederic <fred.konrad@greensocs.com> >>>> >>>> Create virtio-blk which extends virtio-device, so it can be >>>> connected on >>>> virtio-bus. >>>> >>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >>>> --- >>>> hw/virtio-blk.c | 170 >>>> ++++++++++++++++++++++++++++++++++++++++++++++++-------- >>>> hw/virtio-blk.h | 4 ++ >>>> 2 files changed, 150 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c >>>> index e25cc96..ee1ea8b 100644 >>>> --- a/hw/virtio-blk.c >>>> +++ b/hw/virtio-blk.c >>>> @@ -21,24 +21,42 @@ >>>> #ifdef __linux__ >>>> # include <scsi/sg.h> >>>> #endif >>>> +#include "virtio-bus.h" >>>> >>>> +/* Take this structure as our device structure. */ >>>> typedef struct VirtIOBlock >>>> { >>>> + /* >>>> + * Adding parent_obj breaks to_virtio_blk cast function, >>>> + * and virtio_blk_init. >>>> + */ >>>> + DeviceState parent_obj; >>>> + /* >>>> + * We don't need that anymore, as we'll use QOM cast to get the >>>> + * VirtIODevice. Just temporary keep it, for not breaking >>>> functionality. >>>> + */ >>>> VirtIODevice vdev; >>> This doesn't make sense. After your previous patch, VirtIODevice >>> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice, >>> so there's nothing to do here. Adding this parent_obj field >>> here is just breaking things (it would make the VirtIOBlock >>> into a direct child of DeviceState, which isn't what we want). >>> >>>> BlockDriverState *bs; >>>> VirtQueue *vq; >>>> void *rq; >>>> QEMUBH *bh; >>>> BlockConf *conf; >>>> - VirtIOBlkConf *blk; >>>> + /* >>>> + * We can't use pointer with properties. >>>> + */ >>>> + VirtIOBlkConf blk; >>>> unsigned short sector_mask; >>>> DeviceState *qdev; >>>> } VirtIOBlock; >>>> >>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>>> -{ >>>> - return (VirtIOBlock *)vdev; >>>> -} >>>> +/* >>>> + * Use the QOM cast, so we don't need that anymore. >>>> + * >>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>>> + * { >>>> + * return (VirtIOBlock *)vdev; >>>> + * } >>>> + */ >>> If we don't need it, just delete it. >> Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by >> OBJECT_CHECK(), and replace all callers of to_virtio_blk() with >> VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you on. > Yes, that's why I comment to_virtio_blk(). > > Isn't what I made in this patch with : > > +#define TYPE_VIRTIO_BLK "virtio-blk" > +#define VIRTIO_BLK(obj) \ > + OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK) > + > > and > > - VirtIOBlock *s = to_virtio_blk(vdev); > + VirtIOBlock *s = VIRTIO_BLK(vdev); > > ? Sorry. I expected to see the macros above the typedef above, but in the header is even better! :) VIRTIO_BLOCK vs. VIRTIO_BLK is just a style question. Further, I missed on brief sight that the to_* function was commented out, thought it was still being used. Didn't find enough time to review the series fully yet. > I agree with that, but, there is an issue : > The refactored VirtIOBlk is a device and seems to work, but the device > which use this VirtIOBlock > (eg virtio-blk-pci) are just allocating a structure ( in > virtio_common_init ). > > That's why this patch is breaking virtio-blk-pci. Don't understand that part due to lack of virtio knowledge... Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should only be necessary as a command line option alias for backwards compatibility, no? Are you saying you can't make this switch and refactoring for virtio-blk *only*? Andreas
On 06/12/2012 10:53, Andreas Färber wrote: > Am 06.12.2012 10:21, schrieb KONRAD Frédéric: >> On 05/12/2012 18:22, Andreas Färber wrote: >>> Am 05.12.2012 17:25, schrieb Peter Maydell: >>>> On 4 December 2012 14:35, <fred.konrad@greensocs.com> wrote: >>>>> From: KONRAD Frederic <fred.konrad@greensocs.com> >>>>> >>>>> Create virtio-blk which extends virtio-device, so it can be >>>>> connected on >>>>> virtio-bus. >>>>> >>>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com> >>>>> --- >>>>> hw/virtio-blk.c | 170 >>>>> ++++++++++++++++++++++++++++++++++++++++++++++++-------- >>>>> hw/virtio-blk.h | 4 ++ >>>>> 2 files changed, 150 insertions(+), 24 deletions(-) >>>>> >>>>> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c >>>>> index e25cc96..ee1ea8b 100644 >>>>> --- a/hw/virtio-blk.c >>>>> +++ b/hw/virtio-blk.c >>>>> @@ -21,24 +21,42 @@ >>>>> #ifdef __linux__ >>>>> # include <scsi/sg.h> >>>>> #endif >>>>> +#include "virtio-bus.h" >>>>> >>>>> +/* Take this structure as our device structure. */ >>>>> typedef struct VirtIOBlock >>>>> { >>>>> + /* >>>>> + * Adding parent_obj breaks to_virtio_blk cast function, >>>>> + * and virtio_blk_init. >>>>> + */ >>>>> + DeviceState parent_obj; >>>>> + /* >>>>> + * We don't need that anymore, as we'll use QOM cast to get the >>>>> + * VirtIODevice. Just temporary keep it, for not breaking >>>>> functionality. >>>>> + */ >>>>> VirtIODevice vdev; >>>> This doesn't make sense. After your previous patch, VirtIODevice >>>> is-a DeviceState, and VirtIOBlock already is-a VirtIODevice, >>>> so there's nothing to do here. Adding this parent_obj field >>>> here is just breaking things (it would make the VirtIOBlock >>>> into a direct child of DeviceState, which isn't what we want). >>>> >>>>> BlockDriverState *bs; >>>>> VirtQueue *vq; >>>>> void *rq; >>>>> QEMUBH *bh; >>>>> BlockConf *conf; >>>>> - VirtIOBlkConf *blk; >>>>> + /* >>>>> + * We can't use pointer with properties. >>>>> + */ >>>>> + VirtIOBlkConf blk; >>>>> unsigned short sector_mask; >>>>> DeviceState *qdev; >>>>> } VirtIOBlock; >>>>> >>>>> -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>>>> -{ >>>>> - return (VirtIOBlock *)vdev; >>>>> -} >>>>> +/* >>>>> + * Use the QOM cast, so we don't need that anymore. >>>>> + * >>>>> + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) >>>>> + * { >>>>> + * return (VirtIOBlock *)vdev; >>>>> + * } >>>>> + */ >>>> If we don't need it, just delete it. >>> Seconded. You need to introduce a VIRTIO_BLOCK() macro, backed by >>> OBJECT_CHECK(), and replace all callers of to_virtio_blk() with >>> VIRTIO_BLOCK(). Compare my ISA series that I intentionally cc'ed you on. >> Yes, that's why I comment to_virtio_blk(). >> >> Isn't what I made in this patch with : >> >> +#define TYPE_VIRTIO_BLK "virtio-blk" >> +#define VIRTIO_BLK(obj) \ >> + OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK) >> + >> >> and >> >> - VirtIOBlock *s = to_virtio_blk(vdev); >> + VirtIOBlock *s = VIRTIO_BLK(vdev); >> >> ? > Sorry. I expected to see the macros above the typedef above, but in the > header is even better! :) VIRTIO_BLOCK vs. VIRTIO_BLK is just a style > question. > > Further, I missed on brief sight that the to_* function was commented > out, thought it was still being used. Didn't find enough time to review > the series fully yet. Sorry for that, I'll remove the comment. > >> I agree with that, but, there is an issue : >> The refactored VirtIOBlk is a device and seems to work, but the device >> which use this VirtIOBlock >> (eg virtio-blk-pci) are just allocating a structure ( in >> virtio_common_init ). >> >> That's why this patch is breaking virtio-blk-pci. > Don't understand that part due to lack of virtio knowledge... > Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with > this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should > only be necessary as a command line option alias for backwards > compatibility, no? Are you saying you can't make this switch and > refactoring for virtio-blk *only*? I mean that all device which use virtio_blk_init will be broken as we use the OBJECT_CHECK cast and virtio_blk_init currently call virtio_common_init which allocate a VirtIODevice structure ( and don't create any device ). The other virtio-* devices shouldn't be affected. I must test. You suggest an alias for backwards compatibility ? It must create a virtio-pci and a virtio-blk. Is it possible ? Fred > > Andreas >
On 6 December 2012 09:53, Andreas Färber <afaerber@suse.de> wrote: > Am 06.12.2012 10:21, schrieb KONRAD Frédéric: >> I agree with that, but, there is an issue : >> The refactored VirtIOBlk is a device and seems to work, but the device >> which use this VirtIOBlock >> (eg virtio-blk-pci) are just allocating a structure ( in >> virtio_common_init ). >> >> That's why this patch is breaking virtio-blk-pci. > > Don't understand that part due to lack of virtio knowledge... > Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with > this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should > only be necessary as a command line option alias for backwards > compatibility, no? It can't just be a command line alias, or we will break migration. It has to be a simple device that composes together the virtio-pci and virtio-blk devices, plus legacy support for properties and migration state, I think. -- PMM
On 06/12/2012 11:13, Peter Maydell wrote: > On 6 December 2012 09:53, Andreas Färber <afaerber@suse.de> wrote: >> Am 06.12.2012 10:21, schrieb KONRAD Frédéric: >>> I agree with that, but, there is an issue : >>> The refactored VirtIOBlk is a device and seems to work, but the device >>> which use this VirtIOBlock >>> (eg virtio-blk-pci) are just allocating a structure ( in >>> virtio_common_init ). >>> >>> That's why this patch is breaking virtio-blk-pci. >> Don't understand that part due to lack of virtio knowledge... >> Patch 5/6 introduces VirtIODevice as sitting on TYPE_VIRTIO_BUS. So with >> this patch VirtIOBlk is moving to that new bus and virtio-blk-pci should >> only be necessary as a command line option alias for backwards >> compatibility, no? > It can't just be a command line alias, or we will break migration. > It has to be a simple device that composes together the virtio-pci > and virtio-blk devices, plus legacy support for properties and > migration state, I think. > > -- PMM Can we do virtio-blk refactoring and virtio-blk-pci at the same time for not breaking anything ? Or do you have a better idea ? Fred
On 6 December 2012 13:58, KONRAD Frédéric <fred.konrad@greensocs.com> wrote: > On 06/12/2012 11:13, Peter Maydell wrote: >> It can't just be a command line alias, or we will break migration. >> It has to be a simple device that composes together the virtio-pci >> and virtio-blk devices, plus legacy support for properties and >> migration state, I think. > Can we do virtio-blk refactoring and virtio-blk-pci at the same time for not > breaking anything ? Not breaking things is a key part of the requirements here. It's ok to say "I haven't converted virtio-net or the s390 transport in this patchset and therefore they are broken" as an initial RFC (because we can look at how PCI/blk is done and check it works before we expand the same thing out to other transports/devices). But you need to show how the virtio-blk / virtio-pci refactoring works and leaves you with a virtio-blk-pci that isn't broken (either at the end or at any step along the way). -- PMM
On 06/12/2012 15:21, Peter Maydell wrote: > On 6 December 2012 13:58, KONRAD Frédéric <fred.konrad@greensocs.com> wrote: >> On 06/12/2012 11:13, Peter Maydell wrote: >>> It can't just be a command line alias, or we will break migration. >>> It has to be a simple device that composes together the virtio-pci >>> and virtio-blk devices, plus legacy support for properties and >>> migration state, I think. >> Can we do virtio-blk refactoring and virtio-blk-pci at the same time for not >> breaking anything ? > Not breaking things is a key part of the requirements here. Agree with that. > It's ok to say "I haven't converted virtio-net or the s390 > transport in this patchset and therefore they are broken" as > an initial RFC (because we can look at how PCI/blk is done > and check it works before we expand the same thing out to > other transports/devices). But you need to show how the > virtio-blk / virtio-pci refactoring works and leaves you with > a virtio-blk-pci that isn't broken (either at the end or at > any step along the way). And if virtio-blk-pci is broken can we refactor it in the same "patch" ? > > -- PMM >
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index e25cc96..ee1ea8b 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -21,24 +21,42 @@ #ifdef __linux__ # include <scsi/sg.h> #endif +#include "virtio-bus.h" +/* Take this structure as our device structure. */ typedef struct VirtIOBlock { + /* + * Adding parent_obj breaks to_virtio_blk cast function, + * and virtio_blk_init. + */ + DeviceState parent_obj; + /* + * We don't need that anymore, as we'll use QOM cast to get the + * VirtIODevice. Just temporary keep it, for not breaking functionality. + */ VirtIODevice vdev; BlockDriverState *bs; VirtQueue *vq; void *rq; QEMUBH *bh; BlockConf *conf; - VirtIOBlkConf *blk; + /* + * We can't use pointer with properties. + */ + VirtIOBlkConf blk; unsigned short sector_mask; DeviceState *qdev; } VirtIOBlock; -static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) -{ - return (VirtIOBlock *)vdev; -} +/* + * Use the QOM cast, so we don't need that anymore. + * + * static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev) + * { + * return (VirtIOBlock *)vdev; + * } + */ typedef struct VirtIOBlockReq { @@ -55,18 +73,20 @@ typedef struct VirtIOBlockReq static void virtio_blk_req_complete(VirtIOBlockReq *req, int status) { VirtIOBlock *s = req->dev; + VirtIODevice *vdev = VIRTIO_DEVICE(s); trace_virtio_blk_req_complete(req, status); stb_p(&req->in->status, status); virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in)); - virtio_notify(&s->vdev, s->vq); + virtio_notify(vdev, s->vq); } static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error, bool is_read) { - BlockErrorAction action = bdrv_get_error_action(req->dev->bs, is_read, error); + BlockErrorAction action = bdrv_get_error_action(req->dev->bs, is_read, + error); VirtIOBlock *s = req->dev; if (action == BDRV_ACTION_STOP) { @@ -164,7 +184,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req) */ req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base; - if (!req->dev->blk->scsi) { + if (!req->dev->blk.scsi) { status = VIRTIO_BLK_S_UNSUPP; goto fail; } @@ -384,7 +404,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req, * terminated by '\0' only when shorter than buffer. */ strncpy(req->elem.in_sg[0].iov_base, - s->blk->serial ? s->blk->serial : "", + s->blk.serial ? s->blk.serial : "", MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES)); virtio_blk_req_complete(req, VIRTIO_BLK_S_OK); g_free(req); @@ -401,7 +421,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req, static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) { - VirtIOBlock *s = to_virtio_blk(vdev); + VirtIOBlock *s = VIRTIO_BLK(vdev); VirtIOBlockReq *req; MultiReqBuffer mrb = { .num_writes = 0, @@ -422,7 +442,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) static void virtio_blk_dma_restart_bh(void *opaque) { - VirtIOBlock *s = opaque; + VirtIOBlock *s = VIRTIO_BLK(opaque); VirtIOBlockReq *req = s->rq; MultiReqBuffer mrb = { .num_writes = 0, @@ -444,7 +464,7 @@ static void virtio_blk_dma_restart_bh(void *opaque) static void virtio_blk_dma_restart_cb(void *opaque, int running, RunState state) { - VirtIOBlock *s = opaque; + VirtIOBlock *s = VIRTIO_BLK(opaque); if (!running) return; @@ -468,7 +488,7 @@ static void virtio_blk_reset(VirtIODevice *vdev) */ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) { - VirtIOBlock *s = to_virtio_blk(vdev); + VirtIOBlock *s = VIRTIO_BLK(vdev); struct virtio_blk_config blkcfg; uint64_t capacity; int blk_size = s->conf->logical_block_size; @@ -507,7 +527,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config) static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config) { - VirtIOBlock *s = to_virtio_blk(vdev); + VirtIOBlock *s = VIRTIO_BLK(vdev); struct virtio_blk_config blkcfg; memcpy(&blkcfg, config, sizeof(blkcfg)); @@ -516,7 +536,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config) static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features) { - VirtIOBlock *s = to_virtio_blk(vdev); + VirtIOBlock *s = VIRTIO_BLK(vdev); features |= (1 << VIRTIO_BLK_F_SEG_MAX); features |= (1 << VIRTIO_BLK_F_GEOMETRY); @@ -535,7 +555,7 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features) static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status) { - VirtIOBlock *s = to_virtio_blk(vdev); + VirtIOBlock *s = VIRTIO_BLK(vdev); uint32_t features; if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) { @@ -548,10 +568,11 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status) static void virtio_blk_save(QEMUFile *f, void *opaque) { - VirtIOBlock *s = opaque; + VirtIOBlock *s = VIRTIO_BLK(opaque); + VirtIODevice *vdev = VIRTIO_DEVICE(opaque); VirtIOBlockReq *req = s->rq; - virtio_save(&s->vdev, f); + virtio_save(vdev, f); while (req) { qemu_put_sbyte(f, 1); @@ -563,13 +584,14 @@ static void virtio_blk_save(QEMUFile *f, void *opaque) static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id) { - VirtIOBlock *s = opaque; + VirtIOBlock *s = VIRTIO_BLK(opaque); + VirtIODevice *vdev = VIRTIO_DEVICE(opaque); int ret; if (version_id != 2) return -EINVAL; - ret = virtio_load(&s->vdev, f); + ret = virtio_load(vdev, f); if (ret) { return ret; } @@ -591,9 +613,9 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id) static void virtio_blk_resize(void *opaque) { - VirtIOBlock *s = opaque; + VirtIODevice *vdev = VIRTIO_DEVICE(opaque); - virtio_notify_config(&s->vdev); + virtio_notify_config(vdev); } static const BlockDevOps virtio_block_ops = { @@ -630,7 +652,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk) s->vdev.reset = virtio_blk_reset; s->bs = blk->conf.bs; s->conf = &blk->conf; - s->blk = blk; + memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf)); s->rq = NULL; s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; @@ -651,8 +673,108 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk) void virtio_blk_exit(VirtIODevice *vdev) { - VirtIOBlock *s = to_virtio_blk(vdev); + VirtIOBlock *s = VIRTIO_BLK(vdev); unregister_savevm(s->qdev, "virtio-blk", s); blockdev_mark_auto_del(s->bs); virtio_cleanup(vdev); } + +static int virtio_device_init(DeviceState *qdev) +{ + VirtIODevice *vdev = VIRTIO_DEVICE(qdev); + VirtIOBlock *s = VIRTIO_BLK(qdev); + + VirtIOBlkConf *blk = &(s->blk); + static int virtio_blk_id; + + if (!blk->conf.bs) { + error_report("drive property not set"); + return -1; + } + if (!bdrv_is_inserted(blk->conf.bs)) { + error_report("Device needs media, but drive is empty"); + return -1; + } + + blkconf_serial(&blk->conf, &blk->serial); + if (blkconf_geometry(&blk->conf, NULL, 65535, 255, 255) < 0) { + return -1; + } + + virtio_common_init_(vdev, "virtio-blk", VIRTIO_ID_BLOCK, + sizeof(struct virtio_blk_config), + sizeof(VirtIOBlock)); + + vdev->get_config = virtio_blk_update_config; + vdev->set_config = virtio_blk_set_config; + vdev->get_features = virtio_blk_get_features; + vdev->set_status = virtio_blk_set_status; + vdev->reset = virtio_blk_reset; + s->bs = blk->conf.bs; + s->conf = &blk->conf; + memcpy(&(s->blk), blk, sizeof(struct VirtIOBlkConf)); + s->rq = NULL; + s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; + + s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output); + + qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s); + s->qdev = qdev; + register_savevm(qdev, "virtio-blk", virtio_blk_id++, 2, + virtio_blk_save, virtio_blk_load, s); + bdrv_set_dev_ops(s->bs, &virtio_block_ops, s); + bdrv_set_buffer_alignment(s->bs, s->conf->logical_block_size); + + bdrv_iostatus_enable(s->bs); + add_boot_device_path(s->conf->bootindex, qdev, "/disk@0,0"); + + virtio_bus_plug_device(vdev); + + return 0; +} + +static int virtio_device_exit(DeviceState *dev) +{ + VirtIODevice *vdev = VIRTIO_DEVICE(dev); + virtio_blk_exit(vdev); + return 0; +} + +static Property virtio_blk_properties[] = { + DEFINE_BLOCK_PROPERTIES(VirtIOBlock, blk.conf), + DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlock, blk.conf), + DEFINE_PROP_STRING("serial", VirtIOBlock, blk.serial), +#ifdef __linux__ + DEFINE_PROP_BIT("scsi", VirtIOBlock, blk.scsi, 0, true), +#endif + DEFINE_PROP_BIT("config-wce", VirtIOBlock, blk.config_wce, 0, true), + DEFINE_PROP_END_OF_LIST(), +}; + +static void virtio_blk_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass); + dc->init = virtio_device_init; + dc->exit = virtio_device_exit; + dc->props = virtio_blk_properties; + vdc->get_config = virtio_blk_update_config; + vdc->set_config = virtio_blk_set_config; + vdc->get_features = virtio_blk_get_features; + vdc->set_status = virtio_blk_set_status; + vdc->reset = virtio_blk_reset; +} + +static const TypeInfo virtio_device_info = { + .name = TYPE_VIRTIO_BLK, + .parent = TYPE_VIRTIO_DEVICE, + .instance_size = sizeof(VirtIOBlock), + .class_init = virtio_blk_class_init, +}; + +static void virtio_register_types(void) +{ + type_register_static(&virtio_device_info); +} + +type_init(virtio_register_types) diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h index f0740d0..a33336e 100644 --- a/hw/virtio-blk.h +++ b/hw/virtio-blk.h @@ -111,4 +111,8 @@ struct VirtIOBlkConf DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \ DEFINE_PROP_BIT("config-wce", _state, _field, VIRTIO_BLK_F_CONFIG_WCE, true) +#define TYPE_VIRTIO_BLK "virtio-blk" +#define VIRTIO_BLK(obj) \ + OBJECT_CHECK(VirtIOBlock, (obj), TYPE_VIRTIO_BLK) + #endif