On 02/27/2012 11:31 AM, Michael S. Tsirkin wrote:
> On Mon, Feb 27, 2012 at 10:17:21AM +0100, Hannes Reinecke wrote:
[ .. ]
>>
>> The problem with mfi.h is that it's not actually _my_ file, but
>> rather copied over from NetBSD. I felt a bit stupid doing a recoding
>> of all the values which are already present in NetBSD ...
>> Hence the name 'mfi.h', and the different copyright.
>> For the same reason I try to keep the differences between the
>> NetBSD and my version as small as possible.
>>
>> If you have a better solution on how I should handle this
>> I'm willing to listen ...
>
> Document where's the file from as suggested below.
>
Yes, done.
>>>> + if (megasas_use_msix(s) &&
>>>> + msix_init(&s->dev, 15, &s->mmio_io, 0, 0x2000)) {
>>>> + s->flags &= ~MEGASAS_MASK_USE_MSIX;
>>>
>>> You'd want an error message here. maybe even fail init.
>>>
>> But why? The driver continues happily without MSI-X.
>> And the failure message will be printed out via trace events,
>> in case someone is interested.
>
> It is important that the configuration is fully specified
> by the flags.
> For example, otherwise migration to another host where
> MSIX does work will then fail.
>
>
Ok, thanks for the explanation.
But I'll be #ifdef'inf MSI-X support for the time being anyway.
>>>> diff --git a/hw/mfi.h b/hw/mfi.h
>>>> new file mode 100644
>>>> index 0000000..4790c7c
>>>> --- /dev/null
>>>> +++ b/hw/mfi.h
>>>
>>> Sorry if this was discussed already, where is this
>>> code from? freebsd? it seems to have this:
>>> http://gitorious.org/freebsd/freebsd/blobs/HEAD/sys/dev/mfi/mfireg.h
>> Yes, that's the case.
>>
>>> Want to name the file the same and add a link?
>>> This would be an explanation why we keep the
>>> file in a weird style incompatible with qemu.
>>>
>>> Still some things I think are better off fixed.
>>> Noted below.
>>>
>>>> +
>>>> +/* Controller properties */
>>>> +struct mfi_ctrl_props {
>>>> + uint16_t seq_num;
>>>> + uint16_t pred_fail_poll_interval;
>>>> + uint16_t intr_throttle_cnt;
>>>> + uint16_t intr_throttle_timeout;
>>>> + uint8_t rebuild_rate;
>>>> + uint8_t patrol_read_rate;
>>>> + uint8_t bgi_rate;
>>>> + uint8_t cc_rate;
>>>> + uint8_t recon_rate;
>>>> + uint8_t cache_flush_interval;
>>>> + uint8_t spinup_drv_cnt;
>>>> + uint8_t spinup_delay;
>>>> + uint8_t cluster_enable;
>>>> + uint8_t coercion_mode;
>>>> + uint8_t alarm_enable;
>>>> + uint8_t disable_auto_rebuild;
>>>> + uint8_t disable_battery_warn;
>>>> + uint8_t ecc_bucket_size;
>>>> + uint16_t ecc_bucket_leak_rate;
>>>> + uint8_t restore_hotspare_on_insertion;
>>>> + uint8_t expose_encl_devices;
>>>> + uint8_t maintainPdFailHistory;
>>>> + uint8_t disallowHostRequestReordering;
>>>> + uint8_t abortCCOnError;
>>>> + uint8_t loadBalanceMode;
>>>> + uint8_t disableAutoDetectBackplane;
>>>> + uint8_t snapVDSpace;
>>>> + struct {
>>>> + /* set TRUE to disable copyBack (0=copyback enabled) */
>>>> + uint32_t copyBackDisabled:1,
>>>> + SMARTerEnabled:1,
>>>> + prCorrectUnconfiguredAreas:1,
>>>> + useFdeOnly:1,
>>>> + disableNCQ:1,
>>>> + SSDSMARTerEnabled:1,
>>>> + SSDPatrolReadEnabled:1,
>>>> + enableSpinDownUnconfigured:1,
>>>> + autoEnhancedImport:1,
>>>> + enableSecretKeyControl:1,
>>>> + disableOnlineCtrlReset:1,
>>>> + allowBootWithPinnedCache:1,
>>>> + disableSpinDownHS:1,
>>>> + enableJBOD:1,
>>>> + reserved:18;
>>>> + } OnOffProperties;
>>>
>>> Using bitfields for anything where you care about endian-ness
>>> is IMO wrong, and you normally do care for BE host + LE guest.
>>> No idea what bsd does to handle this.
>>>
>> Well, I don't really have a choice. That the firmware interface,
>> which is using this kind of construct.
>> So I'm getting passed in a bitfield, which I then have to evaluate.
>> I should be okay as I'll be doing a byteshuffle before evaluation.
>> But yes, this interface is absolutely horrible.
>
> Bits are pretty comment as an interface.
> The sane thing to do is to have some macros or enums
> specifying the bits. Then you can do
> le32_to_cpu(x) & MFI_DISABLE_SPIN_DOWN_HS
>
Yes, that's a good idea. Will be doing it.
> I don't see the shuffle in code.
> E.g. info->properties.OnOffProperties.enableJBOD = 1
> and no shuffle in sight. Add a comment
> here and where you do the shuffle?
>
Oh. Looks like wishful thinking here.
I _thought_ I did ...
Anyway, will be fixing it up.
Thanks for the review.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
