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

[Don Slutz <Don@CloudSwitch.Com>]

On 09/12/12 09:58, Anthony Liguori wrote:
> Kevin Wolf <kwolf@redhat.com> 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.
Will do in V2.
>>>
>>>> SEABIOS change for booting is in the works.
>>>>
>>>> Tested with Fedora 16, 17.  CentoOS 6. Windows 2003R2 and 2008R2.
>>>>
>>>> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
>>> Minor comments below.
>>>
>>> Coding style does not adhere to qemu standards,
>>> I guess you know that :)
Yes, But I did get checkpatch.pl to not complain.
>>>
>>> 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.
Working on the rewrite.
>> 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?
I have no issue with adding myself to MAINTAINERS (hw/lsilogic*) for 
this patch.  In the near term I will be doing a lot of testing with it.  
I am just starting at looking into some sort of automatic test for this.
> We seem to be growing SCSI controllers like weeds.  Why would someone
> use this verses megasas vs. LSI vs virtio-scsi?
If you are attempting to use a disk that was built under Virtual Box, 
VMware, etc. as a guest for QEMU, it is likely that this is the scsi 
controller (if it was built using one) that was used.
>
> Regards,
>
> Anthony Liguori
>
>> Kevin