hw/scsi/scsi-disk.c | 29 ++++++++++++++++++++++++----- hw/scsi/scsi-generic.c | 48 +++++++++++++++++++++++++++++++++++++----------- include/hw/scsi/scsi.h | 2 ++ 3 files changed, 63 insertions(+), 16 deletions(-)
This is my version of Daniel's patch. In order to keep the RD/WRPROTECT check for emulated SCSI disks, I've added a new property to scsi-disk/hd/cd devices as well. The property, similar to the earlier versions posted by Daniel, is available to the user, but for scsi-disk/hd/cd it affects the INQUIRY value returned by the emulated INQUIRY command. Daniel, can you please test it? Thanks, Paolo Daniel Henrique Barboza (1): hw/scsi: support SCSI-2 passthrough without PI Paolo Bonzini (1): scsi-disk: allow customizing the SCSI version hw/scsi/scsi-disk.c | 29 ++++++++++++++++++++++++----- hw/scsi/scsi-generic.c | 48 +++++++++++++++++++++++++++++++++++++----------- include/hw/scsi/scsi.h | 2 ++ 3 files changed, 63 insertions(+), 16 deletions(-) -- 2.16.2
Hi, On 04/05/2018 01:23 PM, Paolo Bonzini wrote: > This is my version of Daniel's patch. In order to keep the > RD/WRPROTECT check for emulated SCSI disks, I've added a new property > to scsi-disk/hd/cd devices as well. The property, similar to the > earlier versions posted by Daniel, is available to the user, but for > scsi-disk/hd/cd it affects the INQUIRY value returned by the emulated > INQUIRY command. > > Daniel, can you please test it? Emulated case worked as intended: I was able to set the scsi_version of the emulated device via command line. One thing I noticed is that I am also able to set the scsi_version of scsi-block devices - if I set scsi_version=6 in a SCSI passthrough device that has version=2, the version that ends up being passed to the guest is 6. In a quick look I believe that this behavior is caused by setting + s->qdev.scsi_version = s->qdev.default_scsi_version; in scsi_disk_reset, which is the reset function that is also used for scsi-block devices. So, setting scsi_version=6 in the command line overwrites the default_scsi_version = -1 that is being set in patch 2, making scsi_version =! -1 and bypassing the scsi_version assignment code. A quick fix is to get rid of the scsi_version == -1 restriction to set the scsi_version. This is something that Fam proposed in one of the reviews. Given that a well behaved guest will not spam INQUIRY messages (causing the code to always recalculate the same version again), I am fine with it. Another quick fix is to make a new scsi_block_reset function that calls scsi_disk_reset and sets s->scsi_version = -1 right after. Yet another fix is to fully prohibit the user to set scsi_version scsi-block and scsi-generic cases, returning an error message right off the start. Not sure how hard this would be - perhaps the above alternatives are cleaner. Another fix is ... no fix. I am not sure how far the design philosophy of passthrough devices allows the user to overwrite device parameters in despite of their real values, but .... what if the user wants to enforce a scsi_version when using a scsi-block device? The device will surely behave badly, but the user explicitly enforced it via command line, so perhaps let him/her have at it? Thanks, Daniel > > Thanks, > > Paolo > > Daniel Henrique Barboza (1): > hw/scsi: support SCSI-2 passthrough without PI > > Paolo Bonzini (1): > scsi-disk: allow customizing the SCSI version > > hw/scsi/scsi-disk.c | 29 ++++++++++++++++++++++++----- > hw/scsi/scsi-generic.c | 48 +++++++++++++++++++++++++++++++++++++----------- > include/hw/scsi/scsi.h | 2 ++ > 3 files changed, 63 insertions(+), 16 deletions(-) >
On 06/04/2018 01:10, Daniel Henrique Barboza wrote: > Yet another fix is to fully prohibit the user to set scsi_version > scsi-block and scsi-generic cases, returning an error message right off > the start. Not sure how hard this would be - perhaps the above > alternatives are cleaner. > > Another fix is ... no fix. I am not sure how far the design philosophy > of passthrough devices allows the user to overwrite device parameters in > despite of their real values, but .... what if the user wants to > enforce a scsi_version when using a scsi-block device? The device will > surely behave badly, but the user explicitly enforced it via command > line, so perhaps let him/her have at it? Yeah, that was the idea. Removing it is as easy as dropping the DEFINE_PROP_INT32 and initializing to -1 in scsi_block_realize (just like for scsi_generic_realize), but for now we can keep it. My usecase was more to downgrade scsi_version from newer to 2. Paolo
On 04/06/2018 07:12 AM, Paolo Bonzini wrote: > On 06/04/2018 01:10, Daniel Henrique Barboza wrote: >> Yet another fix is to fully prohibit the user to set scsi_version >> scsi-block and scsi-generic cases, returning an error message right off >> the start. Not sure how hard this would be - perhaps the above >> alternatives are cleaner. >> >> Another fix is ... no fix. I am not sure how far the design philosophy >> of passthrough devices allows the user to overwrite device parameters in >> despite of their real values, but .... what if the user wants to >> enforce a scsi_version when using a scsi-block device? The device will >> surely behave badly, but the user explicitly enforced it via command >> line, so perhaps let him/her have at it? > Yeah, that was the idea. Removing it is as easy as dropping the > DEFINE_PROP_INT32 and initializing to -1 in scsi_block_realize (just > like for scsi_generic_realize), but for now we can keep it. My usecase > was more to downgrade scsi_version from newer to 2. I agree. +1 for pulling the patch series as is. Thanks, Daniel > > Paolo >
© 2016 - 2024 Red Hat, Inc.