Re: [Qemu-devel] [RFC PATCH 19/41] hw/block: Request permissions

[Kevin Wolf]

Am 20.02.2017 um 13:25 hat Max Reitz geschrieben:
> On 13.02.2017 18:22, Kevin Wolf wrote:
> > This makes all device emulations with a qdev drive property request
> > permissions on their BlockBackend. We don't block anything yet.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  hw/block/block.c                 | 19 ++++++++++++++++++-
> >  hw/block/fdc.c                   | 25 +++++++++++++++++++++++--
> >  hw/block/m25p80.c                |  8 ++++++++
> >  hw/block/nand.c                  |  7 +++++++
> >  hw/block/nvme.c                  |  8 +++++++-
> >  hw/block/onenand.c               |  7 +++++++
> >  hw/block/pflash_cfi01.c          | 18 ++++++++++++------
> >  hw/block/pflash_cfi02.c          | 19 +++++++++++++------
> >  hw/block/virtio-blk.c            |  8 +++++++-
> >  hw/core/qdev-properties-system.c |  1 -
> >  hw/ide/qdev.c                    |  7 +++++--
> >  hw/nvram/spapr_nvram.c           |  8 ++++++++
> >  hw/scsi/scsi-disk.c              |  8 ++++++--
> 
> Since I have no idea how scsi-generic and all of that works, just an
> innocent question: Do we need to set permissions there, too?

I couldn't see any use for it. With an SG BDS, you can't really do
anything anyway except directly attach it to scsi-generic. And the only
thing that scsi-generic does is invoking ioctls, which the block layer
doesn't understand but just pass though.

So I didn't feel that op blockers could provide anything here.

> >  hw/sd/sd.c                       |  6 ++++++
> >  hw/usb/dev-storage.c             |  6 +++++-
> >  include/hw/block/block.h         |  3 ++-
> >  tests/qemu-iotests/051.pc.out    |  6 +++---
> >  17 files changed, 137 insertions(+), 27 deletions(-)
> 
> [...]
> 
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 2d6eb46..190573c 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -1215,6 +1215,7 @@ static void m25p80_realize(SSISlave *ss, Error **errp)
> >  {
> >      Flash *s = M25P80(ss);
> >      M25P80Class *mc = M25P80_GET_CLASS(s);
> > +    int ret;
> >  
> >      s->pi = mc->pi;
> >  
> > @@ -1222,6 +1223,13 @@ static void m25p80_realize(SSISlave *ss, Error 
> > **errp)
> >      s->dirty_page = -1;
> >  
> >      if (s->blk) {
> > +        uint64_t perm = BLK_PERM_CONSISTENT_READ |
> > +                        (blk_is_read_only(s->blk) ? 0 : BLK_PERM_WRITE);
> > +        ret = blk_set_perm(s->blk, perm, BLK_PERM_ALL, errp);
> 
> Not sure whether it should be changed in this patch, but I don't know
> whether BLK_PERM_ALL is right here; and from a quick glance it doesn't
> look like any of the following patches change it.
> 
> (Same for all of the block device emulation code that invokes
> blk_set_perm() directly.)

Yeah, I'm not completely sure about the real requirements here either.
What I do know is that currently nothing is blocked (so doing the same
in the future won't make things worse at least), and that I don't want
to break exotic devices that I can't really test. So for these devices I
tended to be more permissive in case of doubt.

Kevin

pgp_v8fmwVv8S.pgp
Description: PGP signature