Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LAI misunderstanding of the Preserve field property(?) #144

Open
d-tatianin opened this issue Jan 27, 2024 · 2 comments · May be fixed by #146
Open

LAI misunderstanding of the Preserve field property(?) #144

d-tatianin opened this issue Jan 27, 2024 · 2 comments · May be fixed by #146

Comments

@d-tatianin
Copy link

As indicated by LAI's own field test here:

        OperationRegion (IDXF, SystemIO, 0xC00, 0x2)
        Field (IDXF, ByteAcc, NoLock, Preserve) {
            IDX,   8,
            DAT,   8
        }

        IndexField (IDX, DAT, ByteAcc, NoLock, Preserve) {
            REGA,   8,
            REGB,   8,
            REGC,   8,
            REGD,   8,
            REGE,  16,
        }

        Method(_INI)
        {
            //! io-read: pio 8b 0xC00 = 0x0
            //! io-write: pio 8b 0xC00 = 0x2
            //! io-read: pio 8b 0xC01 = 0x0

            //! io-read: pio 8b 0xC00 = 0x0
            //! io-write: pio 8b 0xC00 = 0x2
            //! io-read: pio 8b 0xC01 = 0x0
            //! io-write: pio 8b 0xC01 = 0xAA
            REGC = 0xAA
            
            ...
        }

Preserve means that all bits outside of a field where field size is less than access size must be preserved. This implies that for field size == access size where field bit offset within byte is 0 no preservation of anything is needed. In the latter case, however, LAI still tries to read data only to discard it later.

I'm also not sure what's up with this top part here:

            //! io-read: pio 8b 0xC00 = 0x0
            //! io-write: pio 8b 0xC00 = 0x2
            //! io-read: pio 8b 0xC01 = 0x0

All in all write to REGC here should cause exactly 2 write accesses and 0 read accesses:

write 2 to IDX
write 0xAA to DAT

ACPICA seems to agree on this at least
@qookei
Copy link
Member

qookei commented Jan 27, 2024

This is caused by the following code unconditionally doing a read to get the original value.

lai/core/opregion.c

Lines 484 to 491 in a228465

if (write_flag == FIELD_PRESERVE) {
if (field->type == LAI_NAMESPACE_FIELD || field->type == LAI_NAMESPACE_BANKFIELD) {
value = lai_perform_read(field->fld_region_node, access_size, offset);
} else if (field->type == LAI_NAMESPACE_INDEXFIELD) {
value = lai_perform_indexfield_read(field, access_size, offset);
} else {
lai_panic("Unknown field type in lai_write_field_internal %d", field->type);
}

One solution would be checking that the write only affects a part of the word, and only doing the read if that is the case.

I'm unsure whether the extra unused read is actually a problem? I suppose it could potentially confuse some hardware that do things on register reads, and it's probably better to stick to what ACPICA does if we want to work on real hardware.

@d-tatianin
Copy link
Author

I'm unsure whether the extra unused read is actually a problem? I suppose it could potentially confuse some hardware that do things on register reads, and it's probably better to stick to what ACPICA does if we want to work on real hardware.

Yeah it's pretty much impossible to say, it might trip up some hardware, might not. There are probably some registers cleared by read? Another thing I thought of is if this was some other address space (aka not SystemMemory or SystemIO) but say SMBus. There a spurious read could cause more problems probably. At the very least it's extra wasted cycles or stalls for no reason.

@qookei qookei mentioned this issue Mar 7, 2024
Open
qookei added a commit to qookei/lai that referenced this issue Mar 9, 2024
@qookei
core/opregion: Avoid needless loads for fields marked Preserve
ea0e030 
Closes managarm#144.
@qookei qookei linked a pull request Mar 9, 2024 that will close this issue
Open
qookei added a commit to qookei/lai that referenced this issue Mar 9, 2024
@qookei
core/opregion: Avoid needless loads for fields marked Preserve
1e0d947 
Closes managarm#144.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

core/opregion: Avoid needless loads for fields marked Preserve qookei/lai
2 participants
@d-tatianin @qookei