On 9/11/24 03:28, Peter Maydell wrote:
> On Wed, 11 Sept 2024 at 07:27, Jacob Abrams <satur9n...@gmail.com> wrote:
>> On 9/10/24 02:34, Peter Maydell wrote:
>>> If we make the .impl and .valid changes, then the result is
>>> that we permit 16 bit writes to come through to the read
>>> and write functions. Since we don't make any changes to
>>> those functions to specially handle size == 2, you get the
>>> effects of the existing code. If the 16 bit write is aligned
>>> to a 4-aligned register offset it will match one of the A_*
>>> cases, and will write 16-bit-value-zero-extended to it.
>>> If the 16 bit write isn't to a 4-aligned offset it will fall
>>> into the "default" case and be logged as a GUEST_ERROR.
>>>
>>> Did I miss some aspect of what the behaviour change is?
>
>> Ah I see your point now regarding the zero extension writes. Basically you
>> are saying this patch will have the effect that a 16-bit write to one of the
>> USART registers such as USART_RTOR via an ARM instruction like STRH would
>> not be handled correctly because value to be written will be zero extended
>> to 32-bits before being written.
>
>> I do believe this is a valid problem. I will test the behavior on my
>> STM32L476 and report back the real HW behavior and I suspect the result
>> would be STRH write to such registers is allowed and unwritten bits are
>> preserved and not overwritten with zeros.
>
> Yeah, the datasheet doesn't say here and so checking the h/w
> behaviour would be helpful.
>
>> I submitted a new patch for just TEACK fix already and will report back on
>> the real HW test result later and adjust 16-bit read/write code as needed to
>> behave properly.
>>
>> I have also submitted a question to my STM contact regarding the reference
>> manual and assuming the answer is yes it is in error I will add comments
>> explaining that to my next patch for the 16-bit read/write handling.
>
> Would certainly be interesting to see what STM say.
> (If the answer is that some of these cases are invalid
> guest behaviour then we have the option of rather than
> following the in-practice h/w behaviour exactly instead
> doing something easy and logging it as a guest-error.)
>
> thanks
> -- PMM
I have received a response from my contact Nicolas Fillon at STM, he wrote "I
see we are making 16 bit access read and write to these 16 bit registers in our
library for both HAL and LL so this should be a documentation issue."
I noticed in the Qemu source the RegisterAccessInfo struct and associated
register_write_memory/register_write functions. These functions appear quite
helpful to ensure that reserved bits are not written by using the ro field. I
don't see very much usage of this paradigm in Qemu however, only Xilinx and USB
DWC3 code appears to use it, but it seems a useful approach in many situations,
especially for STM chips.
On the physical STM hardware, specifically the STM32L476, it allows writes
smaller than 32-bit to 32-bit registers and does not fault or ignore them. In
fact I noticed some very interesting byte duplication behavior, I tested the
following code
// USART_RTOR is a 32-bit register where every bit is r/w
UartHandle.Instance->RTOR = 0xAAAABBBB;
printf("USART1 RTOR 1: 0x%08"PRIx32"\n\r", UartHandle.Instance->RTOR);
uint16_t *rtor_hw = (uint16_t *) &UartHandle.Instance->RTOR;
rtor_hw[1] = (uint16_t) 0xCCCC;
printf("USART1 RTOR 2: 0x%08"PRIx32"\n\r", UartHandle.Instance->RTOR);
uint8_t *rtor_byte = (uint8_t *) &UartHandle.Instance->RTOR;
rtor_byte[3] = (uint8_t) 0xDD;
printf("USART1 RTOR 3: 0x%08"PRIx32"\n\r", UartHandle.Instance->RTOR);
rtor_hw[0] = (uint16_t) 0xEEEE;
printf("USART1 RTOR 4: 0x%08"PRIx32"\n\r", UartHandle.Instance->RTOR);
// USART_BRR is a 32-bit register where only the lower 16-bit are r/w, the
top 16-bits are reserved and read zero
uint32_t *BRR32 = (uint32_t *) &USART2->BRR;
*BRR32 = 0x11223344;
printf("USART2 BRR 1: 0x%08"PRIx32"\n\r", *BRR32);
uint16_t *brr_hw = (uint16_t *) BRR32;
brr_hw[0] = (uint16_t) 0xCCCC;
printf("USART2 BRR 2: 0x%08"PRIx32"\n\r", *BRR32);
uint8_t *brr_byte = (uint8_t *) BRR32;
brr_byte[1] = (uint8_t) 0xDD;
printf("USART2 BRR 3: 0x%08"PRIx32"\n\r", *BRR32);
/*
OUTPUT:
USART1 RTOR 1: 0xaaaabbbb
USART1 RTOR 2: 0xcccccccc
USART1 RTOR 3: 0xdddddddd
USART1 RTOR 4: 0xeeeeeeee
USART2 BRR 1: 0x00003344
USART2 BRR 2: 0x0000cccc
USART2 BRR 3: 0x0000dddd
*/
Notice how an 8-bit write of just 0xDD is duplicates to all words in the
register, strange. I wonder if Qemu is interested in emulating this exact
behavior, it doesn't seem particularly critical since the drivers provided by
STM should never purposely write to fewer bits than are actually writable.
Jacob
