This makes the code slightly safer, also easier to review.
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/sd/sdhci.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index e39623baba..295a89e5d3 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
MASKED_WRITE(s->admaerr, mask, value);
break;
case SDHC_ADMASYSADDR:
- s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL |
- (uint64_t)mask)) | (uint64_t)value;
+ s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value);
break;
case SDHC_ADMASYSADDR + 4:
- s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
- ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
+ s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value);
break;
case SDHC_FEAER:
s->acmd12errsts |= value;
--
2.15.1
On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > This makes the code slightly safer, also easier to review. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/sd/sdhci.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index e39623baba..295a89e5d3 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) > MASKED_WRITE(s->admaerr, mask, value); > break; > case SDHC_ADMASYSADDR: > - s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL | > - (uint64_t)mask)) | (uint64_t)value; > + s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value); This doesn't look right. Originally we were masking admasysaddr with (mask and 0xFFFFFFFF00000000). Then ORing in the value. Now we are depositing value into a bit field that starts at bit 32 and has 0 length. I don't see how value will be deposited at all with a 0 length. Alistair > break; > case SDHC_ADMASYSADDR + 4: > - s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL | > - ((uint64_t)mask << 32))) | ((uint64_t)value << 32); > + s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value); > break; > case SDHC_FEAER: > s->acmd12errsts |= value; > -- > 2.15.1 > >
>> @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>> MASKED_WRITE(s->admaerr, mask, value);
>> break;
>> case SDHC_ADMASYSADDR:
>> - s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL |
>> - (uint64_t)mask)) | (uint64_t)value;
>> + s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value);
>
> This doesn't look right.
>
> Originally we were masking admasysaddr with (mask and
> 0xFFFFFFFF00000000). Then ORing in the value.
>
> Now we are depositing value into a bit field that starts at bit 32 and
> has 0 length. I don't see how value will be deposited at all with a 0
> length.
good catch :) I'll respin with:
case SDHC_ADMASYSADDR:
s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value)
break;
case SDHC_ADMASYSADDR + 4:
s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
break;
>> break;
>> case SDHC_ADMASYSADDR + 4:
>> - s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL |
>> - ((uint64_t)mask << 32))) | ((uint64_t)value << 32);
>> + s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value);
>> break;
>> case SDHC_FEAER:
>> s->acmd12errsts |= value;
On Thu, Dec 14, 2017 at 3:25 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) >>> MASKED_WRITE(s->admaerr, mask, value); >>> break; >>> case SDHC_ADMASYSADDR: >>> - s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL | >>> - (uint64_t)mask)) | (uint64_t)value; >>> + s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value); >> >> This doesn't look right. >> >> Originally we were masking admasysaddr with (mask and >> 0xFFFFFFFF00000000). Then ORing in the value. >> >> Now we are depositing value into a bit field that starts at bit 32 and >> has 0 length. I don't see how value will be deposited at all with a 0 >> length. > > good catch :) I'll respin with: > > case SDHC_ADMASYSADDR: > s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value) > break; > case SDHC_ADMASYSADDR + 4: > s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value); > break; This still doesn't take the mask value into account though. Also, doesn't deposit() shift value up in this case? We want to mask the low bits out. I don't have the code in front of me though, so I could be wrong here. Alistair > >>> break; >>> case SDHC_ADMASYSADDR + 4: >>> - s->admasysaddr = (s->admasysaddr & (0x00000000FFFFFFFFULL | >>> - ((uint64_t)mask << 32))) | ((uint64_t)value << 32); >>> + s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value); >>> break; >>> case SDHC_FEAER: >>> s->acmd12errsts |= value; >
On Thu, Dec 14, 2017 at 8:41 PM, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Thu, Dec 14, 2017 at 3:25 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>> @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) >>>> MASKED_WRITE(s->admaerr, mask, value); >>>> break; >>>> case SDHC_ADMASYSADDR: >>>> - s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL | >>>> - (uint64_t)mask)) | (uint64_t)value; >>>> + s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value); >>> >>> This doesn't look right. >>> >>> Originally we were masking admasysaddr with (mask and >>> 0xFFFFFFFF00000000). Then ORing in the value. >>> >>> Now we are depositing value into a bit field that starts at bit 32 and >>> has 0 length. I don't see how value will be deposited at all with a 0 >>> length. >> >> good catch :) I'll respin with: >> >> case SDHC_ADMASYSADDR: >> s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value) >> break; >> case SDHC_ADMASYSADDR + 4: >> s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value); >> break; > > This still doesn't take the mask value into account though. > > Also, doesn't deposit() shift value up in this case? We want to mask > the low bits out. I don't have the code in front of me though, so I > could be wrong here. We have sdhci_mmio_ops.max_access_size = 4, so value will be at most 32bits. Now ADMASYSADDR is a 64-bit register, accessible in 2x32-bit. /** * Deposit @fieldval into the 64 bit @value at the bit field specified * by the @start and @length parameters, and return the modified * @value. Bits of @value outside the bit field are not modified. uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval); in both access we use length=32 at SDHC_ADMASYSADDR we use start=0, while at SDHC_ADMASYSADDR + 4 we use start=32. both deposit the 32b value (32b masked) into a 64b s->admasysaddr. This is good to clarify this now, because the Spec v3 series (and v4.20 if we want it) add a lot of them. Regards, Phil.
On Thu, Dec 14, 2017 at 4:07 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On Thu, Dec 14, 2017 at 8:41 PM, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> On Thu, Dec 14, 2017 at 3:25 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>>> @@ -1123,12 +1123,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size) >>>>> MASKED_WRITE(s->admaerr, mask, value); >>>>> break; >>>>> case SDHC_ADMASYSADDR: >>>>> - s->admasysaddr = (s->admasysaddr & (0xFFFFFFFF00000000ULL | >>>>> - (uint64_t)mask)) | (uint64_t)value; >>>>> + s->admasysaddr = deposit64(s->admasysaddr, 32, 0, value); >>>> >>>> This doesn't look right. >>>> >>>> Originally we were masking admasysaddr with (mask and >>>> 0xFFFFFFFF00000000). Then ORing in the value. >>>> >>>> Now we are depositing value into a bit field that starts at bit 32 and >>>> has 0 length. I don't see how value will be deposited at all with a 0 >>>> length. >>> >>> good catch :) I'll respin with: >>> >>> case SDHC_ADMASYSADDR: >>> s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value) >>> break; >>> case SDHC_ADMASYSADDR + 4: >>> s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value); >>> break; >> >> This still doesn't take the mask value into account though. >> >> Also, doesn't deposit() shift value up in this case? We want to mask >> the low bits out. I don't have the code in front of me though, so I >> could be wrong here. > > We have sdhci_mmio_ops.max_access_size = 4, so value will be at most 32bits. Ah! Good point. > > Now ADMASYSADDR is a 64-bit register, accessible in 2x32-bit. > > /** > * Deposit @fieldval into the 64 bit @value at the bit field specified > * by the @start and @length parameters, and return the modified > * @value. Bits of @value outside the bit field are not modified. > > uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval); > > in both access we use length=32 > > at SDHC_ADMASYSADDR we use start=0, > while at SDHC_ADMASYSADDR + 4 we use start=32. > > both deposit the 32b value (32b masked) into a 64b s->admasysaddr. > > This is good to clarify this now, because the Spec v3 series (and > v4.20 if we want it) add a lot of them. Ok, this sounds fine to me then. The mask variable is still being ignored though. value should be anded with mask. Alistair > > Regards, > > Phil. >
>>>> good catch :) I'll respin with:
>>>>
>>>> case SDHC_ADMASYSADDR:
>>>> s->admasysaddr = deposit64(s->admasysaddr, 0, 32, value)
>>>> break;
>>>> case SDHC_ADMASYSADDR + 4:
>>>> s->admasysaddr = deposit64(s->admasysaddr, 32, 32, value);
>>>> break;
>>>
>>> This still doesn't take the mask value into account though.
>>>
>>> Also, doesn't deposit() shift value up in this case? We want to mask
>>> the low bits out. I don't have the code in front of me though, so I
>>> could be wrong here.
>>
>> We have sdhci_mmio_ops.max_access_size = 4, so value will be at most 32bits.
>
> Ah! Good point.
>
>> Now ADMASYSADDR is a 64-bit register, accessible in 2x32-bit.
>>
>> /**
>> * Deposit @fieldval into the 64 bit @value at the bit field specified
>> * by the @start and @length parameters, and return the modified
>> * @value. Bits of @value outside the bit field are not modified.
>>
>> uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval);
>>
>> in both access we use length=32
>>
>> at SDHC_ADMASYSADDR we use start=0,
>> while at SDHC_ADMASYSADDR + 4 we use start=32.
>>
>> both deposit the 32b value (32b masked) into a 64b s->admasysaddr.
>>
>> This is good to clarify this now, because the Spec v3 series (and
>> v4.20 if we want it) add a lot of them.
>
> Ok, this sounds fine to me then.
>
> The mask variable is still being ignored though. value should be anded
> with mask.
This is what deposit64() does:
uint64_t deposit64(uint64_t value, int start, int length, uint64_t fieldval)
{
uint64_t mask;
assert(start >= 0 && length > 0 && length <= 64 - start);
mask = (~0ULL >> (64 - length)) << start;
return (value & ~mask) | ((fieldval << start) & mask);
}
© 2016 - 2026 Red Hat, Inc.