| Message ID | 1434406965-30883-2-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/15/2015 04:22 PM, John Snow wrote: > The only guidance the AHCI specification gives on memory access is: > "Register accesses shall have a maximum size of 64-bits; 64-bit access > must not cross an 8-byte alignment boundary." > > In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte reads > regardless of alignment. Windows 7 can also be observed making 1 byte > reads to the middle of 32 bit registers. > > Introduce a wrapper to supper unaligned accesses to AHCI. s/supper/support/ > This wrapper will support aligned 8 byte reads, but will make > no effort to support unaligned 8 byte reads, which although they > will work on real hardware, are not guaranteed to work and do > not appear to be used by either Windows or Linux. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > hw/ide/ahci.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > index 9e5d862..55779fb 100644 > --- a/hw/ide/ahci.c > +++ b/hw/ide/ahci.c > @@ -331,8 +331,7 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val) > } > } > > -static uint64_t ahci_mem_read(void *opaque, hwaddr addr, > - unsigned size) > +static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr) > { > AHCIState *s = opaque; > uint32_t val = 0; > @@ -368,6 +367,24 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr, > } > > > +static uint64_t ahci_mem_read(void *opaque, hwaddr addr, unsigned size) > +{ > + hwaddr aligned = addr & ~0x3; > + int ofst = addr - aligned; > + uint64_t lo = ahci_mem_read_32(opaque, aligned); > + uint64_t hi; > + > + /* if 1/2/4 byte read does not cross 4 byte boundary */ > + if (ofst + size <= 4) { > + return lo >> (ofst * 8); > + } At this point, we could assert(size > 1). > + > + /* If the 64bit read is unaligned, we will produce undefined > + * results. AHCI does not support unaligned 64bit reads. */ > + hi = ahci_mem_read_32(opaque, aligned + 4); > + return (hi << 32) | lo; This makes no effort to support an unaligned 2 byte (16bit) or 4 byte (32bit) read that crosses 4-byte boundary. Is that intentional? I know it is intentional that you don't care about unaligned 64bit reads; conversely, while your commit message mentioned Windows doing 1-byte reads in the middle of 32-bit registers, you didn't mention whether Windows does unaligned 2- or 4-byte reads. So either the comment should be broadened, or the code needs further tuning.
On 06/15/2015 06:55 PM, Eric Blake wrote: > On 06/15/2015 04:22 PM, John Snow wrote: >> The only guidance the AHCI specification gives on memory access is: >> "Register accesses shall have a maximum size of 64-bits; 64-bit access >> must not cross an 8-byte alignment boundary." >> >> In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte reads >> regardless of alignment. Windows 7 can also be observed making 1 byte >> reads to the middle of 32 bit registers. >> >> Introduce a wrapper to supper unaligned accesses to AHCI. > > s/supper/support/ Wow, I guess I'm hungry. > >> This wrapper will support aligned 8 byte reads, but will make >> no effort to support unaligned 8 byte reads, which although they >> will work on real hardware, are not guaranteed to work and do >> not appear to be used by either Windows or Linux. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> hw/ide/ahci.c | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c >> index 9e5d862..55779fb 100644 >> --- a/hw/ide/ahci.c >> +++ b/hw/ide/ahci.c >> @@ -331,8 +331,7 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val) >> } >> } >> >> -static uint64_t ahci_mem_read(void *opaque, hwaddr addr, >> - unsigned size) >> +static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr) >> { >> AHCIState *s = opaque; >> uint32_t val = 0; >> @@ -368,6 +367,24 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr, >> } >> >> >> +static uint64_t ahci_mem_read(void *opaque, hwaddr addr, unsigned size) >> +{ >> + hwaddr aligned = addr & ~0x3; >> + int ofst = addr - aligned; >> + uint64_t lo = ahci_mem_read_32(opaque, aligned); >> + uint64_t hi; >> + >> + /* if 1/2/4 byte read does not cross 4 byte boundary */ >> + if (ofst + size <= 4) { >> + return lo >> (ofst * 8); >> + } > > At this point, we could assert(size > 1). > Sure. I guess in that light my comment above is a little wacky -- 1 byte reads can't cross the boundary ;) >> + >> + /* If the 64bit read is unaligned, we will produce undefined >> + * results. AHCI does not support unaligned 64bit reads. */ >> + hi = ahci_mem_read_32(opaque, aligned + 4); >> + return (hi << 32) | lo; > > This makes no effort to support an unaligned 2 byte (16bit) or 4 byte > (32bit) read that crosses 4-byte boundary. Is that intentional? I know > it is intentional that you don't care about unaligned 64bit reads; > conversely, while your commit message mentioned Windows doing 1-byte > reads in the middle of 32-bit registers, you didn't mention whether > Windows does unaligned 2- or 4-byte reads. So either the comment should > be broadened, or the code needs further tuning. > Good catch. I have not observed any OS making 2 or 4 byte accesses across the register boundary, and cannot think of a reason why you would want to, though the AHCI spec technically doesn't discount your ability to do so and it does work on a real Q35. I can do this: return (hi << 32 | lo) >> (ofst * 8); which will give us unaligned 2 and 4 byte reads, but will really get very wacky for unaligned 8 byte reads -- which you really should probably not be doing anyway.
On 06/15/2015 05:09 PM, John Snow wrote: >> >>> This wrapper will support aligned 8 byte reads, but will make >>> no effort to support unaligned 8 byte reads, which although they >>> will work on real hardware, are not guaranteed to work and do >>> not appear to be used by either Windows or Linux. >>> > >>> + >>> + /* If the 64bit read is unaligned, we will produce undefined >>> + * results. AHCI does not support unaligned 64bit reads. */ >>> + hi = ahci_mem_read_32(opaque, aligned + 4); >>> + return (hi << 32) | lo; >> >> This makes no effort to support an unaligned 2 byte (16bit) or 4 byte >> (32bit) read that crosses 4-byte boundary. Is that intentional? I know >> it is intentional that you don't care about unaligned 64bit reads; >> conversely, while your commit message mentioned Windows doing 1-byte >> reads in the middle of 32-bit registers, you didn't mention whether >> Windows does unaligned 2- or 4-byte reads. So either the comment should >> be broadened, or the code needs further tuning. >> > > Good catch. > Oh, and one other comment - do we care about the contents in the remaining bytes beyond the requested size? > I have not observed any OS making 2 or 4 byte accesses across the > register boundary, and cannot think of a reason why you would want to, > though the AHCI spec technically doesn't discount your ability to do so > and it does work on a real Q35. > > I can do this: > > return (hi << 32 | lo) >> (ofst * 8); > > which will give us unaligned 2 and 4 byte reads, but will really get > very wacky for unaligned 8 byte reads -- which you really should > probably not be doing anyway. I don't mind wacky unaligned 8 byte reads - they are in clear violation of the spec you quoted, so the caller deserves any such garbage they get. But as the spec is silent on unaligned 4 byte reads, and real hardware supports it, it's probably best to support it ourselves even if we haven't observed clients exploiting it. Note that while this returns the desired 16 or 32 bits in the low order side of the result, it does not guarantee that the remaining upper bytes are all 0. I don't know if it matters to callers, or even what real hardware does, but you may want to mask things at both return statements, to guarantee a stable result limited to size bytes of information rather than leaking nearby bytes from the rest of the registers being read.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 06/15/2015 07:28 PM, Eric Blake wrote: > On 06/15/2015 05:09 PM, John Snow wrote: > >>> >>>> This wrapper will support aligned 8 byte reads, but will >>>> make no effort to support unaligned 8 byte reads, which >>>> although they will work on real hardware, are not guaranteed >>>> to work and do not appear to be used by either Windows or >>>> Linux. >>>> > >> >>>> + + /* If the 64bit read is unaligned, we will produce >>>> undefined + * results. AHCI does not support unaligned >>>> 64bit reads. */ + hi = ahci_mem_read_32(opaque, aligned + >>>> 4); + return (hi << 32) | lo; >>> >>> This makes no effort to support an unaligned 2 byte (16bit) or >>> 4 byte (32bit) read that crosses 4-byte boundary. Is that >>> intentional? I know it is intentional that you don't care >>> about unaligned 64bit reads; conversely, while your commit >>> message mentioned Windows doing 1-byte reads in the middle of >>> 32-bit registers, you didn't mention whether Windows does >>> unaligned 2- or 4-byte reads. So either the comment should be >>> broadened, or the code needs further tuning. >>> >> >> Good catch. >> > > Oh, and one other comment - do we care about the contents in the > remaining bytes beyond the requested size? > Do you mean the unmasked higher order bits, if applicable, as you elaborate below? >> I have not observed any OS making 2 or 4 byte accesses across >> the register boundary, and cannot think of a reason why you would >> want to, though the AHCI spec technically doesn't discount your >> ability to do so and it does work on a real Q35. >> >> I can do this: >> >> return (hi << 32 | lo) >> (ofst * 8); >> >> which will give us unaligned 2 and 4 byte reads, but will really >> get very wacky for unaligned 8 byte reads -- which you really >> should probably not be doing anyway. > > I don't mind wacky unaligned 8 byte reads - they are in clear > violation of the spec you quoted, so the caller deserves any such > garbage they get. But as the spec is silent on unaligned 4 byte > reads, and real hardware supports it, it's probably best to support > it ourselves even if we haven't observed clients exploiting it. > > Note that while this returns the desired 16 or 32 bits in the low > order side of the result, it does not guarantee that the remaining > upper bytes are all 0. I don't know if it matters to callers, or > even what real hardware does, but you may want to mask things at > both return statements, to guarantee a stable result limited to > size bytes of information rather than leaking nearby bytes from the > rest of the registers being read. > I believe the masking is handled by the memory system in general, see memory_region_read_accessor, which masks the returned uint64_t with an appropriate value set earlier by access_with_adjusted_size: access_mask = -1ULL >> (64 - access_size * 8); So we're probably OK here, but I can leave a comment if you think it's too hand-wavey. - --js -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJVf2NZAAoJEH3vgQaq/DkOLqwQAItBm26zX4FO3ecRxIBo3QEW DBz+8wCcSHNne1qjiV41ArP+Mc1ckJ8tSob1NohlNekyxN69W5iVd8vsgjfmYZ6t Yqf5Hd5ebEBXMxnNjcCW/pTYQwNitP9RHMuGWnTpRYQxNE2FfV0p8JRSNVk7jsRy /24EmI7ZZv7GhOe4Okh2neiKzizhcxs/XDHrDuJPOkrby73Gc/eCjbXYKGLcLKjh xXrziAPDHtBo4XStNCMWVtSizmLuSbabt6jeoIKIISRyEGwD5v/GFRpXcKsfQlHq Fm/2ITeVFlE5myJLQOV0KLsC6WLtyispgQuMyBXII4+AM2vPkNMc5TdxuYXx1bLt NT+42FYj4lnKqa77SuymsRe+fBUK5FNtJQC1PrdV3gugqUKHVydsQe10Y6me5Ywg OnvGwi5XO4sDePIv7ANGLgtOaNuIZ007Zr+nK43RvTDyby/e2eVkZTEpzt3Lufex zlN8YvjPYM5NwmSXDyKUYICd6Xg6KyFX5lT9KZJ2c6K0cs/bSlD313CQ8l96E+5V Mt+WIvN6ywaJ8q3co4YVkxfbJ+SNPBhkSmlmBnfCH/NwhLtFkTvlMQyqrr4otf3N n+FkLOjAj35jn0oC43plG//aUfTyfkGKFbk7Bw3pBCZqKSf2BT57Fh/iYgXxBw78 Gt+cgknToPRtKWcps0HW =LzUg -----END PGP SIGNATURE-----
On 06/15/2015 05:44 PM, John Snow wrote: >> Note that while this returns the desired 16 or 32 bits in the low >> order side of the result, it does not guarantee that the remaining >> upper bytes are all 0. I don't know if it matters to callers, or >> even what real hardware does, but you may want to mask things at >> both return statements, to guarantee a stable result limited to >> size bytes of information rather than leaking nearby bytes from the >> rest of the registers being read. > > > I believe the masking is handled by the memory system in general, see > memory_region_read_accessor, which masks the returned uint64_t with an > appropriate value set earlier by access_with_adjusted_size: > > access_mask = -1ULL >> (64 - access_size * 8); Good, the caller takes care of it. > > So we're probably OK here, but I can leave a comment if you think it's > too hand-wavey. Yeah, always nicer to state our assumption that we rely on the caller to truncate the result to the desired access size.
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 9e5d862..55779fb 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -331,8 +331,7 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val) } } -static uint64_t ahci_mem_read(void *opaque, hwaddr addr, - unsigned size) +static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr) { AHCIState *s = opaque; uint32_t val = 0; @@ -368,6 +367,24 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr, } +static uint64_t ahci_mem_read(void *opaque, hwaddr addr, unsigned size) +{ + hwaddr aligned = addr & ~0x3; + int ofst = addr - aligned; + uint64_t lo = ahci_mem_read_32(opaque, aligned); + uint64_t hi; + + /* if 1/2/4 byte read does not cross 4 byte boundary */ + if (ofst + size <= 4) { + return lo >> (ofst * 8); + } + + /* If the 64bit read is unaligned, we will produce undefined + * results. AHCI does not support unaligned 64bit reads. */ + hi = ahci_mem_read_32(opaque, aligned + 4); + return (hi << 32) | lo; +} + static void ahci_mem_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
The only guidance the AHCI specification gives on memory access is: "Register accesses shall have a maximum size of 64-bits; 64-bit access must not cross an 8-byte alignment boundary." In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte reads regardless of alignment. Windows 7 can also be observed making 1 byte reads to the middle of 32 bit registers. Introduce a wrapper to supper unaligned accesses to AHCI. This wrapper will support aligned 8 byte reads, but will make no effort to support unaligned 8 byte reads, which although they will work on real hardware, are not guaranteed to work and do not appear to be used by either Windows or Linux. Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)