| Message ID | 4AD5F3BD.2040402@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/14/09 17:52, Naphtali Sprei wrote: > Hi, > as a preliminary step for adding read only flag for the -drive command, I've added code to pass > the (existing) read only attribute of the drive to the guest OS (if it bother to ask). > > I've added for virtio and for scsi. > Where is usb ?? hw/usb-msd.c should be already covered by scsi though. cheers, Gerd
Gerd Hoffmann wrote: > On 10/14/09 17:52, Naphtali Sprei wrote: >> Hi, >> as a preliminary step for adding read only flag for the -drive >> command, I've added code to pass >> the (existing) read only attribute of the drive to the guest OS (if it >> bother to ask). >> >> I've added for virtio and for scsi. > >> Where is usb ?? > > hw/usb-msd.c Thanks, what I meant is I can't find how can it be specified as a drive interface. > > should be already covered by scsi though. > > cheers, > Gerd >
On 10/14/09 17:59, Naphtali Sprei wrote: > Gerd Hoffmann wrote: >> On 10/14/09 17:52, Naphtali Sprei wrote: >>> Hi, >>> as a preliminary step for adding read only flag for the -drive >>> command, I've added code to pass >>> the (existing) read only attribute of the drive to the guest OS (if it >>> bother to ask). >>> >>> I've added for virtio and for scsi. >> >>> Where is usb ?? >> >> hw/usb-msd.c > > Thanks, > what I meant is I can't find how can it be specified as a drive interface. You can use either -usbdevice disk:/some/image (usb_add in monitor) or -drive if=none,id=pendrive,file=/some/image -device usb-storage,drive=pendrive (drive_add + device_add in monitor) cheers, Gerd
Gerd Hoffmann wrote: > On 10/14/09 17:59, Naphtali Sprei wrote: >> Gerd Hoffmann wrote: >>> On 10/14/09 17:52, Naphtali Sprei wrote: >>>> Hi, >>>> as a preliminary step for adding read only flag for the -drive >>>> command, I've added code to pass >>>> the (existing) read only attribute of the drive to the guest OS (if it >>>> bother to ask). >>>> >>>> I've added for virtio and for scsi. >>> >>>> Where is usb ?? >>> >>> hw/usb-msd.c >> >> Thanks, >> what I meant is I can't find how can it be specified as a drive >> interface. > > You can use either > > -usbdevice disk:/some/image > (usb_add in monitor) > > or > > -drive if=none,id=pendrive,file=/some/image > -device usb-storage,drive=pendrive > (drive_add + device_add in monitor) > > cheers, > Gerd > > > Thanks. Verified, it "works", meaning readonly attribute passed to guest.
Naphtali Sprei wrote: <snip> > I'm planning to investigate where qemu should check the read only attribute before exeuting any write command > to drives, would be sent in a different patch. revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting, as long as qemu doesn't crash (or modify the drive). Naphtali
Am 14.10.2009 18:40, schrieb Naphtali Sprei: > Naphtali Sprei wrote: > <snip> >> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command >> to drives, would be sent in a different patch. > > revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting, > as long as qemu doesn't crash (or modify the drive). If the right response to a write on a read-only device is defined in the specification (and it most probably is), we should still give the right response, even though the OS is doing something wrong. Kevin
On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote: > Am 14.10.2009 18:40, schrieb Naphtali Sprei: > > Naphtali Sprei wrote: > > <snip> > >> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command > >> to drives, would be sent in a different patch. > > > > revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting, > > as long as qemu doesn't crash (or modify the drive). > > If the right response to a write on a read-only device is defined in the > specification (and it most probably is), we should still give the right > response, even though the OS is doing something wrong. > And since our response to write error may be pausing a VM we shouldn't allow this to be triggered by a guest OS. -- Gleb.
Am 15.10.2009 11:43, schrieb Gleb Natapov: > On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote: >> Am 14.10.2009 18:40, schrieb Naphtali Sprei: >>> Naphtali Sprei wrote: >>> <snip> >>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command >>>> to drives, would be sent in a different patch. >>> >>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting, >>> as long as qemu doesn't crash (or modify the drive). >> >> If the right response to a write on a read-only device is defined in the >> specification (and it most probably is), we should still give the right >> response, even though the OS is doing something wrong. >> > And since our response to write error may be pausing a VM we shouldn't > allow this to be triggered by a guest OS. I thought we only pause the VM if we get an host IO error? But if you do want to stop it for all errors, you shouldn't start suppressing errors so that it doesn't stop. Kevin
On Thu, Oct 15, 2009 at 11:50:39AM +0200, Kevin Wolf wrote: > Am 15.10.2009 11:43, schrieb Gleb Natapov: > > On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote: > >> Am 14.10.2009 18:40, schrieb Naphtali Sprei: > >>> Naphtali Sprei wrote: > >>> <snip> > >>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command > >>>> to drives, would be sent in a different patch. > >>> > >>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting, > >>> as long as qemu doesn't crash (or modify the drive). > >> > >> If the right response to a write on a read-only device is defined in the > >> specification (and it most probably is), we should still give the right > >> response, even though the OS is doing something wrong. > >> > > And since our response to write error may be pausing a VM we shouldn't > > allow this to be triggered by a guest OS. > > I thought we only pause the VM if we get an host IO error? But if you do > want to stop it for all errors, you shouldn't start suppressing errors > so that it doesn't stop. > We pause only on host IO errors, but if we open underlying file as read only (do we?) and try to write into it we will get an IO error in the host. -- Gleb.
Am 15.10.2009 11:54, schrieb Gleb Natapov: > On Thu, Oct 15, 2009 at 11:50:39AM +0200, Kevin Wolf wrote: >> Am 15.10.2009 11:43, schrieb Gleb Natapov: >>> On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote: >>>> Am 14.10.2009 18:40, schrieb Naphtali Sprei: >>>>> Naphtali Sprei wrote: >>>>> <snip> >>>>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command >>>>>> to drives, would be sent in a different patch. >>>>> >>>>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting, >>>>> as long as qemu doesn't crash (or modify the drive). >>>> >>>> If the right response to a write on a read-only device is defined in the >>>> specification (and it most probably is), we should still give the right >>>> response, even though the OS is doing something wrong. >>>> >>> And since our response to write error may be pausing a VM we shouldn't >>> allow this to be triggered by a guest OS. >> >> I thought we only pause the VM if we get an host IO error? But if you do >> want to stop it for all errors, you shouldn't start suppressing errors >> so that it doesn't stop. >> > We pause only on host IO errors, but if we open underlying file as > read only (do we?) and try to write into it we will get an IO error > in the host. No, we'll return an error before a write request to the host is even issued. Kevin
On Thu, Oct 15, 2009 at 11:55:20AM +0200, Kevin Wolf wrote: > Am 15.10.2009 11:54, schrieb Gleb Natapov: > > On Thu, Oct 15, 2009 at 11:50:39AM +0200, Kevin Wolf wrote: > >> Am 15.10.2009 11:43, schrieb Gleb Natapov: > >>> On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote: > >>>> Am 14.10.2009 18:40, schrieb Naphtali Sprei: > >>>>> Naphtali Sprei wrote: > >>>>> <snip> > >>>>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command > >>>>>> to drives, would be sent in a different patch. > >>>>> > >>>>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting, > >>>>> as long as qemu doesn't crash (or modify the drive). > >>>> > >>>> If the right response to a write on a read-only device is defined in the > >>>> specification (and it most probably is), we should still give the right > >>>> response, even though the OS is doing something wrong. > >>>> > >>> And since our response to write error may be pausing a VM we shouldn't > >>> allow this to be triggered by a guest OS. > >> > >> I thought we only pause the VM if we get an host IO error? But if you do > >> want to stop it for all errors, you shouldn't start suppressing errors > >> so that it doesn't stop. > >> > > We pause only on host IO errors, but if we open underlying file as > > read only (do we?) and try to write into it we will get an IO error > > in the host. > > No, we'll return an error before a write request to the host is even issued. > Who is "we"? If "we" == "bdrv_write()/dma_bdrv_write()" then it's all the same. Upper layers don't actually care why block driver failed. -- Gleb.
Am 15.10.2009 12:01, schrieb Gleb Natapov: > On Thu, Oct 15, 2009 at 11:55:20AM +0200, Kevin Wolf wrote: >> Am 15.10.2009 11:54, schrieb Gleb Natapov: >>> On Thu, Oct 15, 2009 at 11:50:39AM +0200, Kevin Wolf wrote: >>>> Am 15.10.2009 11:43, schrieb Gleb Natapov: >>>>> On Thu, Oct 15, 2009 at 11:36:59AM +0200, Kevin Wolf wrote: >>>>>> Am 14.10.2009 18:40, schrieb Naphtali Sprei: >>>>>>> Naphtali Sprei wrote: >>>>>>> <snip> >>>>>>>> I'm planning to investigate where qemu should check the read only attribute before exeuting any write command >>>>>>>> to drives, would be sent in a different patch. >>>>>>> >>>>>>> revisiting it, if guest OS knows it's a read only device and tries to modify it, anyhow, we don't really care about error reporting, >>>>>>> as long as qemu doesn't crash (or modify the drive). >>>>>> >>>>>> If the right response to a write on a read-only device is defined in the >>>>>> specification (and it most probably is), we should still give the right >>>>>> response, even though the OS is doing something wrong. >>>>>> >>>>> And since our response to write error may be pausing a VM we shouldn't >>>>> allow this to be triggered by a guest OS. >>>> >>>> I thought we only pause the VM if we get an host IO error? But if you do >>>> want to stop it for all errors, you shouldn't start suppressing errors >>>> so that it doesn't stop. >>>> >>> We pause only on host IO errors, but if we open underlying file as >>> read only (do we?) and try to write into it we will get an IO error >>> in the host. >> >> No, we'll return an error before a write request to the host is even issued. >> > Who is "we"? If "we" == "bdrv_write()/dma_bdrv_write()" then it's all the same. > Upper layers don't actually care why block driver failed. Right, "we" is the qemu block layer. If the devices don't use the error code returned, they better should be fixed, I think? Kevin
On Thu, Oct 15, 2009 at 12:05:41PM +0200, Kevin Wolf wrote: > >>>>>> If the right response to a write on a read-only device is defined in the > >>>>>> specification (and it most probably is), we should still give the right > >>>>>> response, even though the OS is doing something wrong. > >>>>>> > >>>>> And since our response to write error may be pausing a VM we shouldn't > >>>>> allow this to be triggered by a guest OS. > >>>> > >>>> I thought we only pause the VM if we get an host IO error? But if you do > >>>> want to stop it for all errors, you shouldn't start suppressing errors > >>>> so that it doesn't stop. > >>>> > >>> We pause only on host IO errors, but if we open underlying file as > >>> read only (do we?) and try to write into it we will get an IO error > >>> in the host. > >> > >> No, we'll return an error before a write request to the host is even issued. > >> > > Who is "we"? If "we" == "bdrv_write()/dma_bdrv_write()" then it's all the same. > > Upper layers don't actually care why block driver failed. > > Right, "we" is the qemu block layer. If the devices don't use the error > code returned, they better should be fixed, I think? > Fixed in what way? There is an option to pause VM on any write error. If block layer returns write error for whatever reason VM will be paused. If scsi/ide/virtio knows that the media is read only it shouldn't issue writes to block layer, but handle it like real HW would. -- Gleb.
Am 15.10.2009 12:11, schrieb Gleb Natapov: > On Thu, Oct 15, 2009 at 12:05:41PM +0200, Kevin Wolf wrote: >>>>>>>> If the right response to a write on a read-only device is defined in the >>>>>>>> specification (and it most probably is), we should still give the right >>>>>>>> response, even though the OS is doing something wrong. >>>>>>>> >>>>>>> And since our response to write error may be pausing a VM we shouldn't >>>>>>> allow this to be triggered by a guest OS. >>>>>> >>>>>> I thought we only pause the VM if we get an host IO error? But if you do >>>>>> want to stop it for all errors, you shouldn't start suppressing errors >>>>>> so that it doesn't stop. >>>>>> >>>>> We pause only on host IO errors, but if we open underlying file as >>>>> read only (do we?) and try to write into it we will get an IO error >>>>> in the host. >>>> >>>> No, we'll return an error before a write request to the host is even issued. >>>> >>> Who is "we"? If "we" == "bdrv_write()/dma_bdrv_write()" then it's all the same. >>> Upper layers don't actually care why block driver failed. >> >> Right, "we" is the qemu block layer. If the devices don't use the error >> code returned, they better should be fixed, I think? >> > Fixed in what way? There is an option to pause VM on any write error. If > block layer returns write error for whatever reason VM will be paused. > If scsi/ide/virtio knows that the media is read only it shouldn't > issue writes to block layer, but handle it like real HW would. Ok, I agree if you want to do it this way. How it's done I really don't care too much, but in the end the guest OS should see the right error. I had the impression that Naphtali and you wanted to silently ignore writes to a read-only disk, which would be just wrong. Kevin
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 3940726..e4511fb 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -631,7 +631,8 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t tag, memset(p, 0, 4); outbuf[1] = 0; /* Default media type. */ outbuf[3] = 0; /* Block descriptor length. */ - if (bdrv_get_type_hint(s->dinfo->bdrv) == BDRV_TYPE_CDROM) { + if (bdrv_get_type_hint(s->dinfo->bdrv) == BDRV_TYPE_CDROM || + bdrv_is_read_only(s->dinfo->bdrv)) { outbuf[2] = 0x80; /* Readonly. */ } p += 4; diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 2630b99..e6df9f2 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -444,6 +444,9 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev) #endif if (strcmp(s->serial_str, "0")) features |= 1 << VIRTIO_BLK_F_IDENTIFY; + + if (bdrv_is_read_only(s->bs)) + features |= 1 << VIRTIO_BLK_F_RO; return features; }
Hi, as a preliminary step for adding read only flag for the -drive command, I've added code to pass the (existing) read only attribute of the drive to the guest OS (if it bother to ask). I've added for virtio and for scsi. Verified it already works for floppy. Searched like mad (in linux sources) for same read only/write protect attribute for ide, but all I found was only for ide-floppy. I assume it's not supported in ide drives. Please correct me if I'm wrong. I don't know about the other interface types for the drive command: sd, mtd, floppy, pflash. Where is usb ?? Will be happy to add those, too, if applicable. Please advise. I'm planning to investigate where qemu should check the read only attribute before exeuting any write command to drives, would be sent in a different patch. Naphtali Signed-off-by: Naphtali Sprei <nsprei@redhat.com> --- hw/scsi-disk.c | 3 ++- hw/virtio-blk.c | 3 +++ 2 files changed, 5 insertions(+), 1 deletions(-)