* [PATCH 0/2] support unaligned access for some xHCI registers @ 2023-12-11 7:12 Tomoyuki HIROSE 2023-12-11 7:12 ` [PATCH 1/2] system/memory.c: support unaligned access Tomoyuki HIROSE ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Tomoyuki HIROSE @ 2023-12-11 7:12 UTC (permalink / raw) To: qemu-devel; +Cc: Tomoyuki HIROSE According to xHCI spec rev 1.2, unaligned access to xHCI Host Controller Capability Registers are not prohibited. But current implementation does not support unaligned access to 'MemoryRegion'. These patches contain 2 changes: 1. support unaligned access to 'MemoryRegion' 2. allow unaligned access to Host Controller Capability Registers. Tomoyuki HIROSE (2): system/memory.c: support unaligned access hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers hw/usb/hcd-xhci.c | 4 +++- system/memory.c | 22 ++++++++++++++++------ 2 files changed, 19 insertions(+), 7 deletions(-) -- 2.39.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] system/memory.c: support unaligned access 2023-12-11 7:12 [PATCH 0/2] support unaligned access for some xHCI registers Tomoyuki HIROSE @ 2023-12-11 7:12 ` Tomoyuki HIROSE 2023-12-11 13:31 ` Cédric Le Goater 2024-01-12 15:48 ` Peter Maydell 2023-12-11 7:12 ` [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers Tomoyuki HIROSE 2023-12-19 4:48 ` [PATCH 0/2] support unaligned access for some xHCI registers Tomoyuki Hirose 2 siblings, 2 replies; 15+ messages in thread From: Tomoyuki HIROSE @ 2023-12-11 7:12 UTC (permalink / raw) To: qemu-devel Cc: Tomoyuki HIROSE, Paolo Bonzini, Peter Xu, David Hildenbrand, Philippe Mathieu-Daudé The previous code ignored 'impl.unaligned' and handled unaligned accesses as is. But this implementation cannot emulate specific registers of some devices that allow unaligned access such as xHCI Host Controller Capability Registers. This commit checks 'impl.unaligned' and if it is false, QEMU emulates unaligned access with multiple aligned access. Signed-off-by: Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> --- system/memory.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/system/memory.c b/system/memory.c index 798b6c0a17..b0caa90fef 100644 --- a/system/memory.c +++ b/system/memory.c @@ -539,6 +539,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, unsigned i; MemTxResult r = MEMTX_OK; bool reentrancy_guard_applied = false; + hwaddr aligned_addr; + unsigned corrected_size = size; + signed align_diff = 0; if (!access_size_min) { access_size_min = 1; @@ -560,18 +563,25 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, reentrancy_guard_applied = true; } - /* FIXME: support unaligned access? */ access_size = MAX(MIN(size, access_size_max), access_size_min); access_mask = MAKE_64BIT_MASK(0, access_size * 8); + if (!mr->ops->impl.unaligned) { + aligned_addr = addr & ~(access_size - 1); + align_diff = addr - aligned_addr; + corrected_size = size < access_size ? access_size : + size + (align_diff > 0 ? access_size : 0); + addr = aligned_addr; + } if (memory_region_big_endian(mr)) { - for (i = 0; i < size; i += access_size) { + for (i = 0; i < corrected_size; i += access_size) { r |= access_fn(mr, addr + i, value, access_size, - (size - access_size - i) * 8, access_mask, attrs); + (size - access_size - i + align_diff) * 8, + access_mask, attrs); } } else { - for (i = 0; i < size; i += access_size) { - r |= access_fn(mr, addr + i, value, access_size, i * 8, - access_mask, attrs); + for (i = 0; i < corrected_size; i += access_size) { + r |= access_fn(mr, addr + i, value, access_size, + ((signed)i - align_diff) * 8, access_mask, attrs); } } if (mr->dev && reentrancy_guard_applied) { -- 2.39.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] system/memory.c: support unaligned access 2023-12-11 7:12 ` [PATCH 1/2] system/memory.c: support unaligned access Tomoyuki HIROSE @ 2023-12-11 13:31 ` Cédric Le Goater 2023-12-15 0:11 ` Tomoyuki Hirose 2024-01-12 15:48 ` Peter Maydell 1 sibling, 1 reply; 15+ messages in thread From: Cédric Le Goater @ 2023-12-11 13:31 UTC (permalink / raw) To: Tomoyuki HIROSE, qemu-devel Cc: Paolo Bonzini, Peter Xu, David Hildenbrand, Philippe Mathieu-Daudé Hello, On 12/11/23 08:12, Tomoyuki HIROSE wrote: > The previous code ignored 'impl.unaligned' and handled unaligned accesses > as is. But this implementation cannot emulate specific registers of some > devices that allow unaligned access such as xHCI Host Controller Capability > Registers. > This commit checks 'impl.unaligned' and if it is false, QEMU emulates > unaligned access with multiple aligned access. > > Signed-off-by: Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> FWIW, there has been a previous proposal for unaligned accesses support [1]. Thanks, C. [1] https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/ > --- > system/memory.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/system/memory.c b/system/memory.c > index 798b6c0a17..b0caa90fef 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -539,6 +539,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, > unsigned i; > MemTxResult r = MEMTX_OK; > bool reentrancy_guard_applied = false; > + hwaddr aligned_addr; > + unsigned corrected_size = size; > + signed align_diff = 0; > > if (!access_size_min) { > access_size_min = 1; > @@ -560,18 +563,25 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, > reentrancy_guard_applied = true; > } > > - /* FIXME: support unaligned access? */ > access_size = MAX(MIN(size, access_size_max), access_size_min); > access_mask = MAKE_64BIT_MASK(0, access_size * 8); > + if (!mr->ops->impl.unaligned) { > + aligned_addr = addr & ~(access_size - 1); > + align_diff = addr - aligned_addr; > + corrected_size = size < access_size ? access_size : > + size + (align_diff > 0 ? access_size : 0); > + addr = aligned_addr; > + } > if (memory_region_big_endian(mr)) { > - for (i = 0; i < size; i += access_size) { > + for (i = 0; i < corrected_size; i += access_size) { > r |= access_fn(mr, addr + i, value, access_size, > - (size - access_size - i) * 8, access_mask, attrs); > + (size - access_size - i + align_diff) * 8, > + access_mask, attrs); > } > } else { > - for (i = 0; i < size; i += access_size) { > - r |= access_fn(mr, addr + i, value, access_size, i * 8, > - access_mask, attrs); > + for (i = 0; i < corrected_size; i += access_size) { > + r |= access_fn(mr, addr + i, value, access_size, > + ((signed)i - align_diff) * 8, access_mask, attrs); > } > } > if (mr->dev && reentrancy_guard_applied) { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] system/memory.c: support unaligned access 2023-12-11 13:31 ` Cédric Le Goater @ 2023-12-15 0:11 ` Tomoyuki Hirose 0 siblings, 0 replies; 15+ messages in thread From: Tomoyuki Hirose @ 2023-12-15 0:11 UTC (permalink / raw) To: Cédric Le Goater Cc: qemu-devel, Paolo Bonzini, Peter Xu, David Hildenbrand, Philippe Mathieu-Daudé On Mon, Dec 11, 2023 at 10:31 PM Cédric Le Goater <clg@redhat.com> wrote: > FWIW, there has been a previous proposal for unaligned accesses support [1]. > > Thanks, > > C. > > [1] https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/ > Thanks for your information. This patch is an RFC, and unfortunately it doesn't seem to have been merged. This feature is useful for devices that allow unaligned access. Regards, Tomoyuki HIROSE On Mon, Dec 11, 2023 at 10:31 PM Cédric Le Goater <clg@redhat.com> wrote: > > Hello, > > On 12/11/23 08:12, Tomoyuki HIROSE wrote: > > The previous code ignored 'impl.unaligned' and handled unaligned accesses > > as is. But this implementation cannot emulate specific registers of some > > devices that allow unaligned access such as xHCI Host Controller Capability > > Registers. > > This commit checks 'impl.unaligned' and if it is false, QEMU emulates > > unaligned access with multiple aligned access. > > > > Signed-off-by: Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> > > FWIW, there has been a previous proposal for unaligned accesses support [1]. > > Thanks, > > C. > > [1] https://lore.kernel.org/qemu-devel/20170630030058.28943-1-andrew@aj.id.au/ > > > > --- > > system/memory.c | 22 ++++++++++++++++------ > > 1 file changed, 16 insertions(+), 6 deletions(-) > > > > diff --git a/system/memory.c b/system/memory.c > > index 798b6c0a17..b0caa90fef 100644 > > --- a/system/memory.c > > +++ b/system/memory.c > > @@ -539,6 +539,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, > > unsigned i; > > MemTxResult r = MEMTX_OK; > > bool reentrancy_guard_applied = false; > > + hwaddr aligned_addr; > > + unsigned corrected_size = size; > > + signed align_diff = 0; > > > > if (!access_size_min) { > > access_size_min = 1; > > @@ -560,18 +563,25 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, > > reentrancy_guard_applied = true; > > } > > > > - /* FIXME: support unaligned access? */ > > access_size = MAX(MIN(size, access_size_max), access_size_min); > > access_mask = MAKE_64BIT_MASK(0, access_size * 8); > > + if (!mr->ops->impl.unaligned) { > > + aligned_addr = addr & ~(access_size - 1); > > + align_diff = addr - aligned_addr; > > + corrected_size = size < access_size ? access_size : > > + size + (align_diff > 0 ? access_size : 0); > > + addr = aligned_addr; > > + } > > if (memory_region_big_endian(mr)) { > > - for (i = 0; i < size; i += access_size) { > > + for (i = 0; i < corrected_size; i += access_size) { > > r |= access_fn(mr, addr + i, value, access_size, > > - (size - access_size - i) * 8, access_mask, attrs); > > + (size - access_size - i + align_diff) * 8, > > + access_mask, attrs); > > } > > } else { > > - for (i = 0; i < size; i += access_size) { > > - r |= access_fn(mr, addr + i, value, access_size, i * 8, > > - access_mask, attrs); > > + for (i = 0; i < corrected_size; i += access_size) { > > + r |= access_fn(mr, addr + i, value, access_size, > > + ((signed)i - align_diff) * 8, access_mask, attrs); > > } > > } > > if (mr->dev && reentrancy_guard_applied) { > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] system/memory.c: support unaligned access 2023-12-11 7:12 ` [PATCH 1/2] system/memory.c: support unaligned access Tomoyuki HIROSE 2023-12-11 13:31 ` Cédric Le Goater @ 2024-01-12 15:48 ` Peter Maydell 2024-01-18 6:43 ` Tomoyuki Hirose 1 sibling, 1 reply; 15+ messages in thread From: Peter Maydell @ 2024-01-12 15:48 UTC (permalink / raw) To: Tomoyuki HIROSE Cc: qemu-devel, Paolo Bonzini, Peter Xu, David Hildenbrand, Philippe Mathieu-Daudé On Mon, 11 Dec 2023 at 07:14, Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> wrote: > > The previous code ignored 'impl.unaligned' and handled unaligned accesses > as is. But this implementation cannot emulate specific registers of some > devices that allow unaligned access such as xHCI Host Controller Capability > Registers. > This commit checks 'impl.unaligned' and if it is false, QEMU emulates > unaligned access with multiple aligned access. > > Signed-off-by: Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> Sorry this has taken me so long to get to reviewing. So, first of all, I think this is definitely a cleaner looking patch than the other one that Cédric posted a link to: if we can have access_with_adjusted_size() cope with the unaligned case that seems nicer than having different functions for the aligned and the unaligned cases. My review comments below are basically about fiddly corner case details. > --- > system/memory.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/system/memory.c b/system/memory.c > index 798b6c0a17..b0caa90fef 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -539,6 +539,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, > unsigned i; > MemTxResult r = MEMTX_OK; > bool reentrancy_guard_applied = false; > + hwaddr aligned_addr; > + unsigned corrected_size = size; > + signed align_diff = 0; > > if (!access_size_min) { > access_size_min = 1; > @@ -560,18 +563,25 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, > reentrancy_guard_applied = true; > } > > - /* FIXME: support unaligned access? */ > access_size = MAX(MIN(size, access_size_max), access_size_min); > access_mask = MAKE_64BIT_MASK(0, access_size * 8); > + if (!mr->ops->impl.unaligned) { > + aligned_addr = addr & ~(access_size - 1); > + align_diff = addr - aligned_addr; > + corrected_size = size < access_size ? access_size : > + size + (align_diff > 0 ? access_size : 0); I don't think this calculation of corrected_size is right for the case when size < access_size. Consider: * size = 2, access_size = 4, addr = 3, little-endian: memory contents from 0 are bytes AA BB CC DD EE FF ... We calculate corrected_size of 4, and we will then do a single 4-byte read of 0xDDCCBBAA. But we need to do two 4-byte reads, because the final result we want to return is 0xEEDD. I think also that we don't really get the optimal behaviour here because we select access_size assuming the aligned case, rather than selecting it specifically for the combination of input size and align_diff in the unaligned case. Consider: access_size_min = 2, access_size_max = 8, size = 4, addr = 2. We'll compute access_size to be 4, and then do the unaligned access with two 4-byte reads. But we could better have done it with two 2-byte reads. This matters especially for the write case, because two 2-byte writes allows us to avoid the problem of "what do we write for the parts of the 4-byte writes that we don't have data from the caller for". (See below for more on that.) > + addr = aligned_addr; > + } > if (memory_region_big_endian(mr)) { > - for (i = 0; i < size; i += access_size) { > + for (i = 0; i < corrected_size; i += access_size) { > r |= access_fn(mr, addr + i, value, access_size, > - (size - access_size - i) * 8, access_mask, attrs); > + (size - access_size - i + align_diff) * 8, > + access_mask, attrs); > } > } else { > - for (i = 0; i < size; i += access_size) { > - r |= access_fn(mr, addr + i, value, access_size, i * 8, > - access_mask, attrs); > + for (i = 0; i < corrected_size; i += access_size) { > + r |= access_fn(mr, addr + i, value, access_size, > + ((signed)i - align_diff) * 8, access_mask, attrs); > } So, with these loops, for unaligned accesses we now load an extra chunk and adjust the shifts so we get the right parts of the chunks we read. However I think we also need to be careful with the access mask for the final access (or the first access in the big-endian case). Consider: * access_size = 2, size = 4, align_diff = 1, little endian, addr = 1 initially (so aligned down to 0), read: and the memory being bytes AA BB CC DD EE FF ... starting at 0. We'll load: * from addr 0, 0xBBAA, which we shift right by 1 for 0xBB * from addr 2, 0xDDCC, shift left 1, for 0xDDCC00 * from addr 4, 0xFFEE, shift left 3, for 0xFFEE000000 and then we OR those together for 0xFFEEDDCCBB so we will have written into *value an extra 0xFF byte that we should not have done. That last access from addr 4 should have an access_mask that says we only want part of it. For writes, things are worse, because we'll do a 2 byte write that writes whatever garbage might have been in the high part of *value. If the device permits an access of smaller size (in this case byte) we can do the end part at that size (or even do the whole write at that size if it's simpler). If the device doesn't permit that smaller size write it's not clear to me what the behaviour should be. (I was going to suggest maybe we should just rule that as not permitted until we run into it, but then your patch 2 puts the xhci-usb device into this category, although harmlessly because it happens to implement writes as ignored.) thanks -- PMM ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] system/memory.c: support unaligned access 2024-01-12 15:48 ` Peter Maydell @ 2024-01-18 6:43 ` Tomoyuki Hirose 0 siblings, 0 replies; 15+ messages in thread From: Tomoyuki Hirose @ 2024-01-18 6:43 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, Paolo Bonzini, Peter Xu, David Hildenbrand, Philippe Mathieu-Daudé Hello, Thank you for reviewing my patches. Examples of corner case you explained is very helpful. I'm currently working on patches. I will submit v2 as soon as it's completed, so please wait a little longer. thanks, Tomoyuki HIROSE ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers 2023-12-11 7:12 [PATCH 0/2] support unaligned access for some xHCI registers Tomoyuki HIROSE 2023-12-11 7:12 ` [PATCH 1/2] system/memory.c: support unaligned access Tomoyuki HIROSE @ 2023-12-11 7:12 ` Tomoyuki HIROSE 2023-12-11 13:57 ` Peter Maydell 2023-12-19 4:48 ` [PATCH 0/2] support unaligned access for some xHCI registers Tomoyuki Hirose 2 siblings, 1 reply; 15+ messages in thread From: Tomoyuki HIROSE @ 2023-12-11 7:12 UTC (permalink / raw) To: qemu-devel; +Cc: Tomoyuki HIROSE, Gerd Hoffmann According to xHCI spec rev 1.2, unaligned access to xHCI Host Controller Capability Registers is not prohibited. In Addition, the limit of access size is also unspecified. Actually, some real devices allow unaligned access and 8-byte access to these registers. This commit makes it possible to unaligned access and 8-byte access to Host Controller Capability Registers. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/143 Signed-off-by: Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> --- hw/usb/hcd-xhci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index 4b60114207..41abeb9ac5 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3181,9 +3181,11 @@ static const MemoryRegionOps xhci_cap_ops = { .read = xhci_cap_read, .write = xhci_cap_write, .valid.min_access_size = 1, - .valid.max_access_size = 4, + .valid.max_access_size = 8, + .valid.unaligned = true, .impl.min_access_size = 4, .impl.max_access_size = 4, + .impl.unaligned = false, .endianness = DEVICE_LITTLE_ENDIAN, }; -- 2.39.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers 2023-12-11 7:12 ` [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers Tomoyuki HIROSE @ 2023-12-11 13:57 ` Peter Maydell 2023-12-12 1:43 ` Tomoyuki Hirose 2024-03-18 16:18 ` Peter Xu 0 siblings, 2 replies; 15+ messages in thread From: Peter Maydell @ 2023-12-11 13:57 UTC (permalink / raw) To: Tomoyuki HIROSE; +Cc: qemu-devel, Gerd Hoffmann On Mon, 11 Dec 2023 at 07:13, Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> wrote: > > According to xHCI spec rev 1.2, unaligned access to xHCI Host > Controller Capability Registers is not prohibited. In Addition, the > limit of access size is also unspecified. Actually, some real devices > allow unaligned access and 8-byte access to these registers. > This commit makes it possible to unaligned access and 8-byte access > to Host Controller Capability Registers. We should definitely look at fixing the unaligned access stuff, but the linked bug report is not trying to do an unaligned access -- it wants to do a 2-byte read from offset 2, which is aligned. The capability registers in the xHCI spec are also all at offsets and sizes that mean that a natural read of them is not unaligned. thanks -- PMM ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers 2023-12-11 13:57 ` Peter Maydell @ 2023-12-12 1:43 ` Tomoyuki Hirose 2023-12-12 10:25 ` Peter Maydell 2024-03-18 16:18 ` Peter Xu 1 sibling, 1 reply; 15+ messages in thread From: Tomoyuki Hirose @ 2023-12-12 1:43 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-devel, Gerd Hoffmann Thanks for comment. On Mon, Dec 11, 2023 at 10:57 PM Peter Maydell <peter.maydell@linaro.org> wrote: > We should definitely look at fixing the unaligned access > stuff, but the linked bug report is not trying to do an > unaligned access -- it wants to do a 2-byte read from offset 2, > which is aligned. The capability registers in the xHCI spec > are also all at offsets and sizes that mean that a natural > read of them is not unaligned. Shouldn't I link this bug report? Or is it not appropriate to allow unaligned access? thanks, Tomoyuki HIROSE ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers 2023-12-12 1:43 ` Tomoyuki Hirose @ 2023-12-12 10:25 ` Peter Maydell 2023-12-18 9:50 ` Tomoyuki Hirose 0 siblings, 1 reply; 15+ messages in thread From: Peter Maydell @ 2023-12-12 10:25 UTC (permalink / raw) To: Tomoyuki Hirose; +Cc: qemu-devel, Gerd Hoffmann On Tue, 12 Dec 2023 at 01:43, Tomoyuki Hirose <tomoyuki.hirose@igel.co.jp> wrote: > > Thanks for comment. > > On Mon, Dec 11, 2023 at 10:57 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > We should definitely look at fixing the unaligned access > > stuff, but the linked bug report is not trying to do an > > unaligned access -- it wants to do a 2-byte read from offset 2, > > which is aligned. The capability registers in the xHCI spec > > are also all at offsets and sizes that mean that a natural > > read of them is not unaligned. > > Shouldn't I link this bug report? > Or is it not appropriate to allow unaligned access? The bug report is definitely relevant. But depending on how tricky the unaligned access handling turns out to be to get right, we might be able to fix the bug by permitting aligned-but-not-4-bytes accesses. (I'm a bit surprised that doesn't work already, in fact: we use it in other devices.) thanks -- PMM ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers 2023-12-12 10:25 ` Peter Maydell @ 2023-12-18 9:50 ` Tomoyuki Hirose 0 siblings, 0 replies; 15+ messages in thread From: Tomoyuki Hirose @ 2023-12-18 9:50 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-devel, Gerd Hoffmann On Tue, Dec 12, 2023 at 7:26 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 12 Dec 2023 at 01:43, Tomoyuki Hirose > <tomoyuki.hirose@igel.co.jp> wrote: > > > > Thanks for comment. > > > > On Mon, Dec 11, 2023 at 10:57 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > We should definitely look at fixing the unaligned access > > > stuff, but the linked bug report is not trying to do an > > > unaligned access -- it wants to do a 2-byte read from offset 2, > > > which is aligned. The capability registers in the xHCI spec > > > are also all at offsets and sizes that mean that a natural > > > read of them is not unaligned. > > > > Shouldn't I link this bug report? > > Or is it not appropriate to allow unaligned access? > > The bug report is definitely relevant. But depending > on how tricky the unaligned access handling turns out to > be to get right, we might be able to fix the bug by > permitting aligned-but-not-4-bytes accesses. (I'm > a bit surprised that doesn't work already, in fact: > we use it in other devices.) > > thanks > -- PMM Thank you for answering my question. The unaligned access handling of my patch is not so tricky. If the access is unaligned, just correct the access size and address and read the value as before. Also, it is allowed by the specifications, and byte access was possible even on real devices. Regards, Tomoyuki HIROSE ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers 2023-12-11 13:57 ` Peter Maydell 2023-12-12 1:43 ` Tomoyuki Hirose @ 2024-03-18 16:18 ` Peter Xu 1 sibling, 0 replies; 15+ messages in thread From: Peter Xu @ 2024-03-18 16:18 UTC (permalink / raw) To: Peter Maydell; +Cc: Tomoyuki HIROSE, qemu-devel, Gerd Hoffmann On Mon, Dec 11, 2023 at 01:57:42PM +0000, Peter Maydell wrote: > On Mon, 11 Dec 2023 at 07:13, Tomoyuki HIROSE > <tomoyuki.hirose@igel.co.jp> wrote: > > > > According to xHCI spec rev 1.2, unaligned access to xHCI Host > > Controller Capability Registers is not prohibited. In Addition, the > > limit of access size is also unspecified. Actually, some real devices > > allow unaligned access and 8-byte access to these registers. > > This commit makes it possible to unaligned access and 8-byte access > > to Host Controller Capability Registers. > > We should definitely look at fixing the unaligned access > stuff, but the linked bug report is not trying to do an > unaligned access -- it wants to do a 2-byte read from offset 2, > which is aligned. IIUC the issue is the xHCI xhci_cap_ops defines impl.min_access_size=4. Then it'll be enlarged to 4 bytes read on offset 0x2. IOW, I think it'll also work if xhci_cap_ops can support impl.min_access_size=2, however I don't know whether that can be more than that register, so that the memory patch is still a more generic approach. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] support unaligned access for some xHCI registers 2023-12-11 7:12 [PATCH 0/2] support unaligned access for some xHCI registers Tomoyuki HIROSE 2023-12-11 7:12 ` [PATCH 1/2] system/memory.c: support unaligned access Tomoyuki HIROSE 2023-12-11 7:12 ` [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers Tomoyuki HIROSE @ 2023-12-19 4:48 ` Tomoyuki Hirose 2023-12-19 11:26 ` Peter Maydell 2 siblings, 1 reply; 15+ messages in thread From: Tomoyuki Hirose @ 2023-12-19 4:48 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Peter Xu, David Hildenbrand, Philippe Mathieu-Daudé, Gerd Hoffmann I would be grateful if you would any comments on my patch. ping, Tomoyuki HIROSE On Mon, Dec 11, 2023 at 4:12 PM Tomoyuki HIROSE <tomoyuki.hirose@igel.co.jp> wrote: > > According to xHCI spec rev 1.2, unaligned access to xHCI Host > Controller Capability Registers are not prohibited. But current > implementation does not support unaligned access to 'MemoryRegion'. > These patches contain 2 changes: > 1. support unaligned access to 'MemoryRegion' > 2. allow unaligned access to Host Controller Capability Registers. > > Tomoyuki HIROSE (2): > system/memory.c: support unaligned access > hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers > > hw/usb/hcd-xhci.c | 4 +++- > system/memory.c | 22 ++++++++++++++++------ > 2 files changed, 19 insertions(+), 7 deletions(-) > > -- > 2.39.2 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] support unaligned access for some xHCI registers 2023-12-19 4:48 ` [PATCH 0/2] support unaligned access for some xHCI registers Tomoyuki Hirose @ 2023-12-19 11:26 ` Peter Maydell 2023-12-20 1:11 ` Tomoyuki Hirose 0 siblings, 1 reply; 15+ messages in thread From: Peter Maydell @ 2023-12-19 11:26 UTC (permalink / raw) To: Tomoyuki Hirose Cc: qemu-devel, Paolo Bonzini, Peter Xu, David Hildenbrand, Philippe Mathieu-Daudé, Gerd Hoffmann On Tue, 19 Dec 2023 at 04:49, Tomoyuki Hirose <tomoyuki.hirose@igel.co.jp> wrote: > > I would be grateful if you would any comments on my patch. It's on my todo list, but at this point I'm afraid I'm not going to be able to get to it before I break for the holidays, so it will be January before I can look at it. (It's a bit complicated because I'll need to look at this patch, at the other suggested patch from the past that also tried to address this, at various mailing list discussions we've had about the problem, and at how the code in general is doing things, so it's not a trivial change to review.) thanks -- PMM ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] support unaligned access for some xHCI registers 2023-12-19 11:26 ` Peter Maydell @ 2023-12-20 1:11 ` Tomoyuki Hirose 0 siblings, 0 replies; 15+ messages in thread From: Tomoyuki Hirose @ 2023-12-20 1:11 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, Paolo Bonzini, Peter Xu, David Hildenbrand, Philippe Mathieu-Daudé, Gerd Hoffmann On Tue, Dec 19, 2023 at 8:26 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 19 Dec 2023 at 04:49, Tomoyuki Hirose > <tomoyuki.hirose@igel.co.jp> wrote: > > > > I would be grateful if you would any comments on my patch. > > It's on my todo list, but at this point I'm afraid I'm > not going to be able to get to it before I break for > the holidays, so it will be January before I can look at > it. (It's a bit complicated because I'll need to look > at this patch, at the other suggested patch from the > past that also tried to address this, at various > mailing list discussions we've had about the problem, > and at how the code in general is doing things, so it's > not a trivial change to review.) OK, I understand. Enjoy your holidays! Regards, Tomoyuki HIROSE ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-03-18 16:19 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-11 7:12 [PATCH 0/2] support unaligned access for some xHCI registers Tomoyuki HIROSE 2023-12-11 7:12 ` [PATCH 1/2] system/memory.c: support unaligned access Tomoyuki HIROSE 2023-12-11 13:31 ` Cédric Le Goater 2023-12-15 0:11 ` Tomoyuki Hirose 2024-01-12 15:48 ` Peter Maydell 2024-01-18 6:43 ` Tomoyuki Hirose 2023-12-11 7:12 ` [PATCH 2/2] hw/usb/hcd-xhci.c: allow unaligned access to Capability Registers Tomoyuki HIROSE 2023-12-11 13:57 ` Peter Maydell 2023-12-12 1:43 ` Tomoyuki Hirose 2023-12-12 10:25 ` Peter Maydell 2023-12-18 9:50 ` Tomoyuki Hirose 2024-03-18 16:18 ` Peter Xu 2023-12-19 4:48 ` [PATCH 0/2] support unaligned access for some xHCI registers Tomoyuki Hirose 2023-12-19 11:26 ` Peter Maydell 2023-12-20 1:11 ` Tomoyuki Hirose
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).