On 11/24/15 23:48, Kinney, Michael D wrote:
> Laszlo,
>
> Thanks for the detailed feedback. Comments included below. V2 patch to
> follow.
>
> Mike
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Tuesday, November 24, 2015 11:57 AM
>> To: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-de...@ml01.01.org
>> Cc: Yao, Jiewen <jiewen....@intel.com>; Fan, Jeff <jeff....@intel.com>
>> Subject: Re: [edk2] [PATCH 2/2] UefiCpuPkg:CpuS3DataDxe - Add module to
>> initialize ACPI_CPU_DATA for S3
>>
>> On 11/24/15 07:24, Michael Kinney wrote:
[snip]
>>> +
>>> + //
>>> + // Get the boot processor's GDT and IDT
>>> + //
>>> + AsmReadGdtr ((IA32_DESCRIPTOR *)&AcpiCpuDataEx->GdtrProfile);
>>> + AsmReadIdtr ((IA32_DESCRIPTOR *)&AcpiCpuDataEx->IdtrProfile);
>>
>> (15) I believe you don't need the explicit type casts.
>
> The type casts are required because GdrtProfile and IdtrProfile fields are
> type EFI_PHYSICAL_ADDRESS
The following fields are of type EFI_PHYSICAL_ADDRESS:
- AcpiCpuDataEx->AcpuCpuData.GdtrProfile
- AcpiCpuDataEx->AcpuCpuData.IdtrProfile
The following fields have type IA32_DESCRIPTOR:
- AcpiCpuDataEx->GdtrProfile
- AcpiCpuDataEx->IdtrProfile
Therefore in the above you don't need the casts. You would need the
casts if you wrote:
AsmReadGdtr (
(IA32_DESCRIPTOR *)(UINTN)AcpiCpuDataEx->AcpuCpuData.GdtrProfile
);
Or, identically:
AsmReadGdtr ((IA32_DESCRIPTOR *)(UINTN)AcpiCpuData->GdtrProfile);
But, it is a minor comment anyway.
By the way, I just noticed that there is a typo in the ACPI_CPU_DATA_EX
structure: the field in question is currently called Acp[u]CpuData.
I haven't looked at v2 yet. If the above two non-issues are present in
v2 as well, they can be removed in v3.
Or else, if v3 turns out otherwise unnecessary, the superfluous casts
and the typo in the field name can be removed when committing v2; if you
think it's worthwhile.
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
