Re: acpi_resource bug?
On Sunday, February 13, 2011 2:46:07 pm Matthew Fleming wrote: I'm not very familiar with the acpi code, but we have seen an intermittent issue on boot: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? It should just use res-Length: Index: acpi_resource.c === --- acpi_resource.c (revision 218554) +++ acpi_resource.c (working copy) @@ -82,7 +82,7 @@ acpi_lookup_irq_handler(ACPI_RESOURCE *res, void * req-found = 1; KASSERT(irq == rman_get_start(req-res), (IRQ resources do not match)); - bcopy(res, req-acpi_res, sizeof(ACPI_RESOURCE)); + bcopy(res, req-acpi_res, res-Length); return (AE_CTRL_TERMINATE); } return (AE_OK); -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: acpi_resource bug?
On Mon, Feb 14, 2011 at 6:24 AM, John Baldwin j...@freebsd.org wrote: On Sunday, February 13, 2011 2:46:07 pm Matthew Fleming wrote: I'm not very familiar with the acpi code, but we have seen an intermittent issue on boot: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? It should just use res-Length: Is there a guarantee that res-Length is = sizeof(ACPI_RESOURCE) ? Thanks, matthew Index: acpi_resource.c === --- acpi_resource.c (revision 218554) +++ acpi_resource.c (working copy) @@ -82,7 +82,7 @@ acpi_lookup_irq_handler(ACPI_RESOURCE *res, void * req-found = 1; KASSERT(irq == rman_get_start(req-res), (IRQ resources do not match)); - bcopy(res, req-acpi_res, sizeof(ACPI_RESOURCE)); + bcopy(res, req-acpi_res, res-Length); return (AE_CTRL_TERMINATE); } return (AE_OK); -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: acpi_resource bug?
On 02/14/11 10:29, Matthew Fleming wrote: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? It should just use res-Length: Is there a guarantee that res-Length is = sizeof(ACPI_RESOURCE) ? I don't know if it's related or a different bug .. If I run 'acpidump -t', I get a core-dump and .. [ .. snip .. ] /* MCFG: Length=60, Revision=1, Checksum=74, OEMID=INTEL, OEM Table ID=CALISTGA, OEM Revision=0x604, Creator ID=LOHR, Creator Revision=0x5a Base Address=0xe000 Segment Group=0x Start Bus=0 End Bus=255 */ /* TCPA: Length=50, Revision=1, Checksum=153, OEMID=PTLTD, OEM Table ID=CALISTGA, OEM Revision=0x604, Creator ID= PTL, Creator Revision=0x1 Class 0 Base Address 0x0 Length 65536 -268370093 0xc3e200f053ff00f053ff00f054ff00f0de9100f0 [unknown 0xf000ff53] ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: acpi_resource bug?
On Monday 14 February 2011 10:29 am, Matthew Fleming wrote:
On Mon, Feb 14, 2011 at 6:24 AM, John Baldwin j...@freebsd.org
wrote:
On Sunday, February 13, 2011 2:46:07 pm Matthew Fleming wrote:
I'm not very familiar with the acpi code, but we have seen an
intermittent issue on boot:
1) should the length of the bcopy() be changed to either respect
res-Length or the actual length of the ACPI_RESOURCE_DATA for
the type?
It should just use res-Length:
Is there a guarantee that res-Length is = sizeof(ACPI_RESOURCE) ?
No. Please try the attached patch (after your r218685).
Jung-uk Kim
Index: sys/dev/acpica/acpi_resource.c
===
--- sys/dev/acpica/acpi_resource.c (revision 218686)
+++ sys/dev/acpica/acpi_resource.c (working copy)
@@ -65,31 +65,30 @@ acpi_lookup_irq_handler(ACPI_RESOURCE *res, void *
switch (res-Type) {
case ACPI_RESOURCE_TYPE_IRQ:
+ irqnum = res-Data.Irq.InterruptCount;
+ irq = res-Data.Irq.Interrupts[0];
+ len = ACPI_RS_SIZE(ACPI_RESOURCE_IRQ);
+ break;
case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
- if (res-Type == ACPI_RESOURCE_TYPE_IRQ) {
- irqnum = res-Data.Irq.InterruptCount;
- irq = res-Data.Irq.Interrupts[0];
- } else {
- irqnum = res-Data.ExtendedIrq.InterruptCount;
- irq = res-Data.ExtendedIrq.Interrupts[0];
- }
- if (irqnum != 1)
- break;
- req = (struct lookup_irq_request *)context;
- if (req-counter != req-rid) {
- req-counter++;
- break;
- }
- req-found = 1;
- KASSERT(irq == rman_get_start(req-res),
- (IRQ resources do not match));
- len = res-Length;
- if (len sizeof(ACPI_RESOURCE))
- len = sizeof(ACPI_RESOURCE);
- bcopy(res, req-acpi_res, len);
- return (AE_CTRL_TERMINATE);
+ irqnum = res-Data.ExtendedIrq.InterruptCount;
+ irq = res-Data.ExtendedIrq.Interrupts[0];
+ len = ACPI_RS_SIZE(ACPI_RESOURCE_EXTENDED_IRQ);
+ break;
+default:
+ return (AE_OK);
}
-return (AE_OK);
+if (irqnum != 1)
+ return (AE_OK);
+req = (struct lookup_irq_request *)context;
+if (req-counter != req-rid) {
+ req-counter++;
+ return (AE_OK);
+}
+req-found = 1;
+KASSERT(irq == rman_get_start(req-res),
+ (IRQ resources do not match));
+bcopy(res, req-acpi_res, len);
+return (AE_CTRL_TERMINATE);
}
ACPI_STATUS
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: acpi_resource bug?
On Monday, February 14, 2011 1:30:18 pm Jung-uk Kim wrote: On Monday 14 February 2011 10:29 am, Matthew Fleming wrote: On Mon, Feb 14, 2011 at 6:24 AM, John Baldwin j...@freebsd.org wrote: On Sunday, February 13, 2011 2:46:07 pm Matthew Fleming wrote: I'm not very familiar with the acpi code, but we have seen an intermittent issue on boot: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? It should just use res-Length: Is there a guarantee that res-Length is = sizeof(ACPI_RESOURCE) ? No. Please try the attached patch (after your r218685). I think your patch is correct, but are you saying that ACPICA will return a resource with a size that doesn't match its type? ACPI_RESOURCE_DATA is a union of all the various resource types, and it does contain both ACPI_RESOURCE_IRQ and ACPI_RESOURCE_EXTENDED_IRQ, so it's hard to see how res-Length would be greater than the size of ACPI_RESOURCE. -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: acpi_resource bug?
On Monday 14 February 2011 10:37 am, Michael Butler wrote: On 02/14/11 10:29, Matthew Fleming wrote: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? It should just use res-Length: Is there a guarantee that res-Length is = sizeof(ACPI_RESOURCE) ? I don't know if it's related or a different bug .. If I run 'acpidump -t', I get a core-dump and .. [ .. snip .. ] /* MCFG: Length=60, Revision=1, Checksum=74, OEMID=INTEL, OEM Table ID=CALISTGA, OEM Revision=0x604, Creator ID=LOHR, Creator Revision=0x5a Base Address=0xe000 Segment Group=0x Start Bus=0 End Bus=255 */ /* TCPA: Length=50, Revision=1, Checksum=153, OEMID=PTLTD, OEM Table ID=CALISTGA, OEM Revision=0x604, Creator ID= PTL, Creator Revision=0x1 Class 0 Base Address 0x0 Length 65536 -268370093 0xc3e200f053ff00f053ff00f054ff00f0de9100f0 [unknown 0xf000ff53] No, I don't think it is not related. Our acpidump(8) has its own table parser. Jung-uk Kim ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: acpi_resource bug?
On Mon, Feb 14, 2011 at 10:37 AM, John Baldwin j...@freebsd.org wrote: On Monday, February 14, 2011 1:30:18 pm Jung-uk Kim wrote: On Monday 14 February 2011 10:29 am, Matthew Fleming wrote: On Mon, Feb 14, 2011 at 6:24 AM, John Baldwin j...@freebsd.org wrote: On Sunday, February 13, 2011 2:46:07 pm Matthew Fleming wrote: I'm not very familiar with the acpi code, but we have seen an intermittent issue on boot: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? It should just use res-Length: Is there a guarantee that res-Length is = sizeof(ACPI_RESOURCE) ? No. Please try the attached patch (after your r218685). I think your patch is correct, but are you saying that ACPICA will return a resource with a size that doesn't match its type? ACPI_RESOURCE_DATA is a union of all the various resource types, and it does contain both ACPI_RESOURCE_IRQ and ACPI_RESOURCE_EXTENDED_IRQ, so it's hard to see how res-Length would be greater than the size of ACPI_RESOURCE. Jung-uk Kim's patch makes acpi_resource match the other bcopy's in the acpica directory, and in the case of what I saw it would bcopy 8+5 bytes instead of res-Length == 16. My concern was that res-Length seemed primarily to be an offset from the current resource to the next one, and I didn't see why that may not include a lot of padding (including more than the target of the bcopy was prepared for). However, my code will also copy bytes we don't care about any time res-Length is rounded up from the actual struct size. The patch looks fine to me. I don't have direct access to the machine that was intermittently crashing so I can't really try the new patch, but my change resolved the issue on it. Thanks, matthew ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: acpi_resource bug?
On Monday 14 February 2011 01:37 pm, John Baldwin wrote: On Monday, February 14, 2011 1:30:18 pm Jung-uk Kim wrote: On Monday 14 February 2011 10:29 am, Matthew Fleming wrote: On Mon, Feb 14, 2011 at 6:24 AM, John Baldwin j...@freebsd.org wrote: On Sunday, February 13, 2011 2:46:07 pm Matthew Fleming wrote: I'm not very familiar with the acpi code, but we have seen an intermittent issue on boot: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? It should just use res-Length: Is there a guarantee that res-Length is = sizeof(ACPI_RESOURCE) ? No. Please try the attached patch (after your r218685). I think your patch is correct, but are you saying that ACPICA will return a resource with a size that doesn't match its type? ACPI_RESOURCE_DATA is a union of all the various resource types, and it does contain both ACPI_RESOURCE_IRQ and ACPI_RESOURCE_EXTENDED_IRQ, so it's hard to see how res-Length would be greater than the size of ACPI_RESOURCE. Some resource type has variable size. For example, ACPI_RESOURCE_EXTENDED_IRQ has ACPI_RESOURCE_SOURCE field, which has a pointer to string and Length field will include length of the string (+ 1 for null terminator). Also, the Length only includes the ACPI_RESOURCE_DATA, not the header itself, etc. Jung-uk Kim ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: acpi_resource bug?
On Monday 14 February 2011 01:40 pm, Jung-uk Kim wrote: On Monday 14 February 2011 10:37 am, Michael Butler wrote: On 02/14/11 10:29, Matthew Fleming wrote: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? It should just use res-Length: Is there a guarantee that res-Length is = sizeof(ACPI_RESOURCE) ? I don't know if it's related or a different bug .. If I run 'acpidump -t', I get a core-dump and .. [ .. snip .. ] /* MCFG: Length=60, Revision=1, Checksum=74, OEMID=INTEL, OEM Table ID=CALISTGA, OEM Revision=0x604, Creator ID=LOHR, Creator Revision=0x5a Base Address=0xe000 Segment Group=0x Start Bus=0 End Bus=255 */ /* TCPA: Length=50, Revision=1, Checksum=153, OEMID=PTLTD, OEM Table ID=CALISTGA, OEM Revision=0x604, Creator ID= PTL, Creator Revision=0x1 Class 0 Base Address 0x0 Length 65536 -268370093 0xc3e200f053ff00f053ff00f054ff00f0de9100f0 [unknown 0xf000ff53] No, I don't think it is not related. Our acpidump(8) has its own table parser. A terrible typo, sorry. I meant it is not related and our TCPA table parser seems incomplete. Jung-uk Kim ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: acpi_resource bug?
On Monday 14 February 2011 01:45 pm, Matthew Fleming wrote: On Mon, Feb 14, 2011 at 10:37 AM, John Baldwin j...@freebsd.org wrote: On Monday, February 14, 2011 1:30:18 pm Jung-uk Kim wrote: On Monday 14 February 2011 10:29 am, Matthew Fleming wrote: On Mon, Feb 14, 2011 at 6:24 AM, John Baldwin j...@freebsd.org wrote: On Sunday, February 13, 2011 2:46:07 pm Matthew Fleming wrote: I'm not very familiar with the acpi code, but we have seen an intermittent issue on boot: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? It should just use res-Length: Is there a guarantee that res-Length is = sizeof(ACPI_RESOURCE) ? No. �Please try the attached patch (after your r218685). I think your patch is correct, but are you saying that ACPICA will return a resource with a size that doesn't match its type? ACPI_RESOURCE_DATA is a union of all the various resource types, and it does contain both ACPI_RESOURCE_IRQ and ACPI_RESOURCE_EXTENDED_IRQ, so it's hard to see how res-Length would be greater than the size of ACPI_RESOURCE. Jung-uk Kim's patch makes acpi_resource match the other bcopy's in the acpica directory, and in the case of what I saw it would bcopy 8+5 bytes instead of res-Length == 16. Actually, it is 8+6 bytes, i.e., sizeof(ACPI_RESOURCE_IRQ) == 6. ;-) Also, res-Length does not include header but it includes padding bytes for alignment. My concern was that res-Length seemed primarily to be an offset from the current resource to the next one, and I didn't see why that may not include a lot of padding (including more than the target of the bcopy was prepared for). However, my code will also copy bytes we don't care about any time res-Length is rounded up from the actual struct size. Correct. The patch looks fine to me. I don't have direct access to the machine that was intermittently crashing so I can't really try the new patch, but my change resolved the issue on it. I'll go ahead and commit my patch if you don't have any more concerns. Thanks for working on this, Jung-uk Kim ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
acpi_resource bug?
I'm not very familiar with the acpi code, but we have seen an intermittent issue on boot: Panic occurred in module kernel loaded at 0x8010: Stack: -- kernel:trap_fatal+0xac kernel:trap_pfault+0x24c kernel:trap+0x42e kernel:bcopy+0x16 kernel:AcpiWalkResources+0xdf kernel:acpi_lookup_irq_resource+0x9e kernel:acpi_alloc_resource+0x249 kernel:bus_alloc_resource+0x97 kernel:sioattach+0x446 kernel:device_attach+0x63 kernel:bus_generic_attach+0x27 kernel:acpi_probe_children+0x50 kernel:acpi_attach+0x836 kernel:device_attach+0x63 kernel:bus_generic_attach+0x27 kernel:nexus_attach+0x25 kernel:device_attach+0x63 kernel:root_bus_configure+0x2d kernel:configure+0x1a kernel:mi_startup+0x64 -- cpuid = 0; apic id = 00 fault virtual address = 0xff8003abe000 fault code = supervisor read data, page not present acpi_lookup_irq_handler() is trying to bcopy an entire ACPI_RESOURCE (68 bytes) from the input pointer, even though in this case the resource was a ACPI_RESOURCE_TYPE_IRQ (5 bytes), and the loop in AcpiWalkResourcessaw is seeing res-Length == 0x10. In this case, I found the following resouces on the list: (gdb) x/2wx 0xff8003abdfb0 0xff8003abdfb0: 0x0004 0x0010 (gdb) x/2wx 0xff8003abdfc0 0xff8003abdfc0: 0x0004 0x0010 (gdb) x/2wx 0xff8003abdfd0 0xff8003abdfd0: 0x 0x0010 (gdb) x/2wx 0xff8003abdfe0 0xff8003abdfe0: 0x0001 0x0010 (gdb) x/2wx 0xff8003abdff0 0xff8003abdff0: 0x0007 0x0010 So copying 68 bytes from 0xff8003abdfd0 will always fault. What I wonder is the following: 1) should the length of the bcopy() be changed to either respect res-Length or the actual length of the ACPI_RESOURCE_DATA for the type? 2) why would there be no memory mapped at the next virtual page on some boots, but not others? I *think* that a reboot doesn't clear the issue, but booting into a different kernel with no relevant changes will change whether the panic on boot is hit. Thanks, matthew ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
