[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 01/10] hw/sd.c: convert wp_groups in SDState
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH V2 01/10] hw/sd.c: convert wp_groups in SDState to bitfield |
Date: |
Wed, 11 Apr 2012 13:15:47 +0100 |
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
[Qemu-devel] [PATCH V2 03/10] hw/sd.c: make sd_dataready() return bool, Igor Mitsyanko, 2012/04/05
[Qemu-devel] [PATCH V2 07/10] SD card: introduce "if-idx" property for SD card objects, Igor Mitsyanko, 2012/04/05
[Qemu-devel] [PATCH V2 05/10] hw/sd.c: add SD card save/load support, Igor Mitsyanko, 2012/04/05
[Qemu-devel] [PATCH V2 10/10] hw/sd.c: introduce SD card "image" property and allow SD hot-insert, Igor Mitsyanko, 2012/04/05