Re: [Qemu-devel] [PATCH] hw: Add support for new LSI Logic devices.

[Anthony Liguori]

Kevin Wolf <address@hidden> writes:

> Am 12.09.2012 01:50, schrieb Michael S. Tsirkin:
>> On Tue, Sep 11, 2012 at 01:00:13PM -0400, Don Slutz wrote:
>>> Add LSI53C1030, SAS1068, SAS1068e.  Based on code from "VirtualBox Open 
>>> Source Edition".
>>> Based on QEMU MegaRAID SAS 8708EM2.
>>>
>>> This is a common VMware disk controller.
>> 
>> I think you mean VMware emulates this controller too,
>> pls say it explicitly in the commit log.
>> 
>>> SEABIOS change for booting is in the works.
>>>
>>> Tested with Fedora 16, 17.  CentoOS 6. Windows 2003R2 and 2008R2.
>>>
>>> Signed-off-by: Don Slutz <address@hidden>
>> 
>> Minor comments below.
>> 
>> Coding style does not adhere to qemu standards,
>> I guess you know that :)
>> 
>> Otherwise, from pci side of things this looks OK.
>> I did not look at the scsi side of things.
>> 
>>> ---
>>>  default-configs/pci.mak |    1 +
>>>  hw/Makefile.objs        |    1 +
>>>  hw/lsilogic.c           | 2743 ++++++++++++++++++++++++++++++++++++++
>>>  hw/lsilogic.h           | 3365 
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>  hw/pci_ids.h            |    4 +
>>>  trace-events            |   26 +
>>>  6 files changed, 6140 insertions(+), 0 deletions(-)
>>>  create mode 100644 hw/lsilogic.c
>>>  create mode 100644 hw/lsilogic.h
>>>
>>> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
>>> index 69e18f1..ae4873d 100644
>>> --- a/default-configs/pci.mak
>>> +++ b/default-configs/pci.mak
>>> @@ -11,6 +11,7 @@ CONFIG_PCNET_PCI=y
>>>  CONFIG_PCNET_COMMON=y
>>>  CONFIG_LSI_SCSI_PCI=y
>>>  CONFIG_MEGASAS_SCSI_PCI=y
>>> +CONFIG_LSILOGIC_SCSI_PCI=y
>>>  CONFIG_RTL8139_PCI=y
>>>  CONFIG_E1000_PCI=y
>>>  CONFIG_IDE_CORE=y
>>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>>> index 6dfebd2..e5f939c 100644
>>> --- a/hw/Makefile.objs
>>> +++ b/hw/Makefile.objs
>>> @@ -115,6 +115,7 @@ hw-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
>>>  # SCSI layer
>>>  hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
>>>  hw-obj-$(CONFIG_MEGASAS_SCSI_PCI) += megasas.o
>>> +hw-obj-$(CONFIG_LSILOGIC_SCSI_PCI) += lsilogic.o
>>>  hw-obj-$(CONFIG_ESP) += esp.o
>>>  hw-obj-$(CONFIG_ESP_PCI) += esp-pci.o
>>>  
>>> diff --git a/hw/lsilogic.c b/hw/lsilogic.c
>>> new file mode 100644
>>> index 0000000..1c0a54f
>>> --- /dev/null
>>> +++ b/hw/lsilogic.c
>>> @@ -0,0 +1,2743 @@
>>> +/*
>>> + * QEMU LSILOGIC LSI53C1030 SCSI and SAS1068 Host Bus Adapter emulation
>>> + * Based on the QEMU Megaraid emulator and the VirtualBox LsiLogic
>>> + * LSI53c1030 SCSI controller
>>> + *
>>> + * Copyright (c) 2009-2012 Hannes Reinecke, SUSE Labs
>>> + *
>>> + * This library is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU Lesser General Public
>>> + * License as published by the Free Software Foundation; either
>>> + * version 2 of the License, or (at your option) any later version.
>>> + *
>>> + * This library is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * Lesser General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU Lesser General Public
>>> + * License along with this library; if not, see 
>>> <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +/* Id: DevLsiLogicSCSI.cpp 40642 2012-03-26 13:14:08Z vboxsync $ */
>>> +/** @file
>>> + * VBox storage devices: LsiLogic LSI53c1030 SCSI controller.
>>> + */
>>> +
>>> +/*
>>> + * Copyright (C) 2006-2009 Oracle Corporation
>>> + *
>>> + * This file is part of VirtualBox Open Source Edition (OSE), as
>>> + * available from http://www.virtualbox.org. This file is free software;
>>> + * you can redistribute it and/or modify it under the terms of the GNU
>>> + * General Public License (GPL) as published by the Free Software
>>> + * Foundation, in version 2 as it comes in the "COPYING" file of the
>>> + * VirtualBox OSE distribution. VirtualBox OSE is distributed in the
>>> + * hope that it will be useful, but WITHOUT ANY WARRANTY of any kind.
>>> + */
>>> +
>>> +
>> 
>> I suspect you need to rewrite above: probably add
>> all copyrights in 1st header and make it v2 only.
>
> Do we even accept new GPLv2-only code?

Yes.

I've got some concern about the maintainability of this though.  This is
code copied from another project and then heavily modified.

Are we prepared to independently fork this device?  How are we going to test it
regularly?

We seem to be growing SCSI controllers like weeds.  Why would someone
use this verses megasas vs. LSI vs virtio-scsi?

Regards,

Anthony Liguori

>
> Kevin