Re: [Qemu-devel] [PATCH][v14] megasas: LSI Megaraid SAS HBA emulation

[Hannes Reinecke <hare@suse.de>]

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
hare@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)