Le lun. 16 déc. 2019 20:46, Niek Linnenbank <nieklinnenb...@gmail.com> a écrit :
> > > On Mon, Dec 16, 2019 at 1:14 AM Philippe Mathieu-Daudé <phi...@redhat.com> > wrote: > >> On 12/16/19 12:07 AM, Niek Linnenbank wrote: >> > >> > >> > On Fri, Dec 13, 2019 at 12:56 AM Philippe Mathieu-Daudé >> > <phi...@redhat.com <mailto:phi...@redhat.com>> wrote: >> > >> > Hi Niek, >> > >> > On 12/11/19 11:34 PM, Niek Linnenbank wrote: >> [...] >> > > +static uint32_t aw_h3_sdhost_process_desc(AwH3SDHostState >> *s, >> > > + hwaddr desc_addr, >> > > + TransferDescriptor >> > *desc, >> > > + bool is_write, >> > uint32_t >> > > max_bytes) >> > > +{ >> > > + uint32_t num_done = 0; >> > > + uint32_t num_bytes = max_bytes; >> > > + uint8_t buf[1024]; >> > > + >> > > + /* Read descriptor */ >> > > + cpu_physical_memory_read(desc_addr, desc, >> sizeof(*desc)); >> > >> > Should we worry about endianess here? >> > >> > >> > I tried to figure out what is expected, but the >> > Allwinner_H3_Datasheet_V1.2.pdf does not >> > explicitly mention endianness for any of its I/O devices. Currently it >> > seems all devices are >> > happy with using the same endianness as the CPUs. In the >> MemoryRegionOps >> > has DEVICE_NATIVE_ENDIAN >> > set to match the behavior seen. >> >> OK. >> >> [...] >> > > +static const MemoryRegionOps aw_h3_sdhost_ops = { >> > > + .read = aw_h3_sdhost_read, >> > > + .write = aw_h3_sdhost_write, >> > > + .endianness = DEVICE_NATIVE_ENDIAN, >> > >> > I haven't checked .valid accesses from the datasheet. >> > >> > However due to: >> > >> > res = s->data_crc[((offset - REG_SD_DATA7_CRC) / >> sizeof(uint32_t))]; >> > >> > You seem to expect: >> > >> > .impl.min_access_size = 4, >> > >> > .impl.max_access_size unset is 8, which should works. >> > >> > It seems that all registers are aligned on at least 32-bit boundaries. >> > And the section 5.3.5.1 mentions >> > that the DMA descriptors must be stored in memory 32-bit aligned. So >> > based on that information, >> >> So you are describing ".valid.min_access_size = 4", which is the minimum >> access size on the bus. >> ".impl.min_access_size" is different, it is what access sizes is ready >> to handle your model. >> Your model read/write handlers expect addresses aligned on 32-bit >> boundary, this is why I suggested to use ".impl.min_access_size = 4". If >> the guest were using a 16-bit access, your model would be buggy. If you >> describe your implementation to accept minimum 32-bit and the guest is >> allowed to use smaller accesses, QEMU will do a 32-bit access to the >> device, and return the 16-bit part to the guest. This way your model is >> safe. This is done by access_with_adjusted_size() in memory.c. >> If you restrict with ".valid.min_access_size = 4", you might think we >> don't need ".valid.min_access_size = 4" because all access from guest >> will be at least 32-bit. However keep in mind someone might find this >> device in another datasheet not limited to 32-bit, and let's say change >> to ".valid.min_access_size = 2". Without ".impl.min_access_size = 4" >> your model is buggy. So to be safe I'd use: >> >> .impl.min_access_size = 4, >> .valid.min_access_size = 4, >> > > Now it makes more sense to me, thanks Philippe for explaining this! > Great, I'll add .impl.min_access_size = 4. > > At this point, I've processed all the feedback that I received for all of > the patches > in this series. Is there anything else you would like to > see/discuss/review, or shall I send the v2 when I finish testing? > Send it! We'll discuss on updated v2 :) Regards, Phil.
