From owner-freebsd-virtualization@FreeBSD.ORG Wed Mar 27 07:17:09 2013 Return-Path: Delivered-To: freebsd-virtualization@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 90591E8F for ; Wed, 27 Mar 2013 07:17:09 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: from mail-ea0-x22e.google.com (mail-ea0-x22e.google.com [IPv6:2a00:1450:4013:c01::22e]) by mx1.freebsd.org (Postfix) with ESMTP id 30596941 for ; Wed, 27 Mar 2013 07:17:09 +0000 (UTC) Received: by mail-ea0-f174.google.com with SMTP id m14so1081990eaj.19 for ; Wed, 27 Mar 2013 00:17:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:sender:message-id:date:from:user-agent:mime-version:to :subject:content-type:content-transfer-encoding; bh=2swH8qV9u3AKBowjws2G5LJgWK+Y8lhp7DQbZhQIthY=; b=kArz5li4OL64Xh0KHu23Lxo3XI9PFHtcIE0juIy3CVCNoaENPjWEO7soWi5yL//w2h xftNxTi0dCezhI8XH9No+JCv8CERW0r9Ydv80YKVpdyxcHwzt9+X4ObYVcMesK1+vM5B tQKaJEtfDHsQNfyurRzfxeVWfSWW0mPPXRX4wpypgK9BaeKaMlKkOhqt/WCXkt/2jEQ4 M5HRUdkPIfBDgGtFZ9pZkozH1lQCSl0YmHpHk7l2RZUrgmIoniLUS/6BigMnDJPQlnlK 2OBQXFafrUNXzaurvqQ07oNnPG16rrL1msk0DBZrTgFto4b73lGN5bwlNLimWE8nrZm9 tQeQ== X-Received: by 10.15.36.67 with SMTP id h43mr52477238eev.5.1364368628344; Wed, 27 Mar 2013 00:17:08 -0700 (PDT) Received: from mavbook.mavhome.dp.ua (mavhome.mavhome.dp.ua. [213.227.240.37]) by mx.google.com with ESMTPS id t4sm29353059eel.0.2013.03.27.00.17.05 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 27 Mar 2013 00:17:07 -0700 (PDT) Sender: Alexander Motin Message-ID: <51529CF0.5090503@FreeBSD.org> Date: Wed, 27 Mar 2013 09:17:04 +0200 From: Alexander Motin User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:17.0) Gecko/20130326 Thunderbird/17.0.4 MIME-Version: 1.0 To: Larry Melia , freebsd-virtualization@freebsd.org Subject: Re: New vendor branch for FreeBSD on Hyper-V. Please review. Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-virtualization@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Discussion of various virtualization techniques FreeBSD supports." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Mar 2013 07:17:09 -0000 Hi. As ATA driver maintainer I've took a look on that part of the code. I have some comments about what I see. Please keep me CC'd, as I am normally not on this list. 1) You are using machine dependent function do_cpuid() in machine independent ATA code. It will just not build on any architecture except amd64 and i386. You should periodically run `make universe` to be sure that you code does not breaks the build for some rare archs. As minimum that code should be wrapped into respective #ifdef's, but I would prefer it to be moved out somewhere to MD code (like MD code that does general VM detection now), as this doesn't really belong to the ATA driver. 2) You are disabling "ad" ATA disk driver. Here I have two objections: a) "ad" driver is obsoleted and not used since FreeBSD 9.0, when CAM-based ATA stack replaces it completely, wrapping only controller driver parts of old ata(4) into CAM SIM, which is now only one of four ATA/SATA SIMs in the system. Your change is effectively null now. b) I believe you are using wrong approach here. I believe instead of completely disabling disk driver you should disable only specific Hyper-V emulated ATA controller device instance. I've never used Hyper-V, but I guess it should support PCI device pass-through. Just imagine what happen if somebody wish to pass-through ATA/SATA controller? You will disable it also. 3) Looking on what I suppose is "Fast IDE" driver in the file hv_storvsc_drv_freebsd.c, it seems more like cut-down SAS, then IDE. It doesn't support ATA commands, it reports itself to CAM as SAS. The only differences I see are disabling LUNs and limiting queue to 1 slot. Just a smile: it is neither really "Fast" nor at all "IDE". :) 4) In you SAS driver in storvsc_action() you have hardcoded CTS_SCSI_FLAGS_TAG_ENB flag. While I guess it should not cause any problems, it wont allow user to control it if he decide to experiment with disabling it. -- Alexander Motin