Re: [Qemu-devel] [PATCH V2 01/10] hw/sd.c: convert wp_groups in SDState to bitfield

[Peter Maydell]

On 11 April 2012 12:57, Igor Mitsyanko <address@hidden> wrote:
> On 04/11/2012 02:12 PM, Peter Maydell wrote:
>>
>> On 5 April 2012 16:48, Igor Mitsyanko<address@hidden>  wrote:
>>>
>>> @@ -536,8 +541,8 @@ static void sd_function_switch(SDState *sd, uint32_t
>>> arg)
>>>
>>>  static inline int sd_wp_addr(SDState *sd, uint32_t addr)
>>>  {
>
>
> I've just noticed that it truncates addr to 32 bits... And test_bit() and
> bitmap_new() takes int argument. Do we care about this?

We definitely don't want to restrict the SD card image size to
32 bits, so any byte addresses must be uint64_t, and you're
correct that sd_wp_addr is wrong here.

I don't think we care that the bitmap size is limited to
possibly be 32 bits only on a 32 bit system, because there's
only one bit per wp group, and so it doesn't impose much of a
restriction on the file size.

>>> -    return sd->wp_groups[addr>>
>>> -            (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT)];
>>> +    return test_bit(addr>>  (HWBLOCK_SHIFT + SECTOR_SHIFT +
>>> WPGROUP_SHIFT),
>>> +            sd->wp_groups);
>>
>>
>> Looking at how often the expression "addr>>  (HWBLOCK_SHIFT +
>> SECTOR_SHIFT + WPGROUP_SHIFT)"
>> turns up in this file, I suspect that it would be helpful to have
>> a function
>> static uint32_t addr_to_wpnum(uint64_t addr) {
>>     return addr>>  (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
>> }
>>
>
> This implicitly limits max address to 0xFFFFFFFF << (HWBLOCK_SHIFT +
> SECTOR_SHIFT + WPGROUP_SHIFT), have you done this on purpose?

You could argue for uint64_t return type, but we have to pass
the result to the bitmap functions anyway, so there is implicitly
a limit. I don't think it's a particularly severe one.

If you'd rather uint64_t return here I'm happy with that: I guess
that means the sd.c code isn't putting any extra limits beyond
what the bitmap code is, so it's probably better.

-- PMM