Date: Wed, 27 Mar 2013 09:17:04 +0200 From: Alexander Motin <mav@FreeBSD.org> To: Larry Melia <larry.melia@ieee.org>, freebsd-virtualization@freebsd.org Subject: Re: New vendor branch for FreeBSD on Hyper-V. Please review. Message-ID: <51529CF0.5090503@FreeBSD.org>
next in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?51529CF0.5090503>