Re: [Qemu-block] [PATCH V4 3/3] virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device

[Michael S. Tsirkin]

On Mon, Jul 27, 2015 at 03:28:51PM +0200, Cornelia Huck wrote:
> On Mon, 27 Jul 2015 14:22:37 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Mon, Jul 27, 2015 at 12:30:19PM +0200, Paolo Bonzini wrote:
> > > 
> > > 
> > > On 27/07/2015 11:49, Jason Wang wrote:
> > > > So this patch only clear VIRTIO_F_LAYOUT for legacy device.
> > > > 
> > > > Cc: Stefan Hajnoczi <address@hidden>
> > > > Cc: Kevin Wolf <address@hidden>
> > > > Cc: address@hidden
> > > > Signed-off-by: Jason Wang <address@hidden>
> > > > ---
> > > >  hw/block/virtio-blk.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > index 9acbc3a..1d3f26c 100644
> > > > --- a/hw/block/virtio-blk.c
> > > > +++ b/hw/block/virtio-blk.c
> > > > @@ -731,7 +731,6 @@ static uint64_t 
> > > > virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
> > > >      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
> > > >      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
> > > >      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> > > > -    virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > > >      if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
> > > >          if (s->conf.scsi) {
> > > >              error_setg(errp, "Virtio 1.0 does not support scsi 
> > > > passthrough!");
> > > > @@ -739,6 +738,7 @@ static uint64_t 
> > > > virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
> > > >          }
> > > >          virtio_add_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > > >      } else {
> > > > +        virtio_clear_feature(&features, VIRTIO_F_ANY_LAYOUT);
> > > >          virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
> > > >      }
> > > 
> > > This patch is unnecessary, since the feature is added back below under
> > > "if (__virtio_has_feature(features, VIRTIO_F_VERSION_1))".
> > > 
> > > Paolo
> > 
> > It's needed so we can apply
> >     virtio: set any_layout in virtio core
> 
> So what's the plan on all those virtio feature patches? It's hard to
> keep track about what is based upon what, and what the end result looks
> like.

I pushed everything out to my pci branch, pls take a look.
This is what I have:

b787b35 acpi: fix pvpanic device is not shown in ui
49009db hw/acpi/ich9: clean up stale comment about KVM not supporting SMM
e513e9c hw/acpi/ich9: clear smi_en on reset
c9b11f9 virtio-blk: only clear VIRTIO_F_ANY_LAYOUT for legacy device
efb8206 virtio-blk: fail get_features when both scsi and 1.0 were set
9d5b731 virtio: get_features() can fail
2746269 virtio-pci: fix memory MR cleanup for modern
09999a5 virtio: set any_layout in virtio core
cd4bfbb virtio-9p: fix any_layout
7882080 virtio-serial: fix ANY_LAYOUT
5f45607 virtio: hide legacy features from modern guests


> I don't have a good feeling about doing this that late in the 2.4
> cycle.

Well there will always be bugs. Given modern is disabled by default,
even if more bugs surface after release it's not the end of the
world.

-- 
MST