Laszlo, very sorry for that, it was my mistake that missing your name. when I reply mail, I copy the "CC" list to the mail reply list, but forget to copy the "To" list. I will check your comments in detailed later and reply you. thanks again. On 2017/5/30 0:03, Laszlo Ersek wrote: > Hi, > > did you remove me from the To: / Cc: list intentionally, or was that an > oversight? I caught your message in my list folders only by luck. > > Some followup below: > > On 05/29/17 17:27, gengdongjiu wrote: > >>> (46) What is "physical_addr" good for? Below I can only see an >>> assignment to it, in ghes_update_guest(). Where is the field read? > >> this "physical_addr" address is the physical error address in the >> CPER. such as the physical address that happen hwpoison, this address >> is delivered by the KVM and QEMU transfer this address to physical. > I understand that in the ghes_update_guest() function, you accept a > parameter called "physical_address", and you pass it on to > ghes_generate_cper_record(). That makes sense, yes. > > However, you also assign the same value to "ges.physical_addr". And that > structure field is never read. So my point is that the > "GhesErrorState.physical_addr" field is superfluous and should be removed. > > I checked the other three patches in the series and they don't seem to > read that structure member either. Correct me if I'm wrong. > >>> (55) What happens if you run out of the preallocated memory? > >> if it run out of the preallocated memory. it will overwrite other >> error source. every block's size is fixed. so it does not easy >> dynamically extend the size if it is overflow. Anyway I will add a >> error report if it happens overwrite. > I understand (and agree) that dynamic allocation is not possible here. > > But that doesn't justify overwriting the error status data block that > belongs to a different data source. (Worse, if this happens with the > last error status data block, for error source 10, you could overwrite > memory that belongs to the OS.) > > If an error status data block becomes full, then we should either wrap > back to the start of the same data block, or else stop forwarding errors > for that error source. > > Does the ACPI spec say anything about this? I.e., about the case when > the system runs out of the memory that was reserved for recording > hardware errors? > >>>>> + >>>>> + mem_err = (struct cper_sec_mem_err *) (gdata + 1); >>>>> + >>>>> + /* In order to simplify simulation, hardcode the CPER section to memory >>>>> + * section. >>>>> + */ >>>>> + mem_err->validation_bits |= CPER_MEM_VALID_ERROR_TYPE; >>>>> + mem_err->error_type = 3; >>> >>> (58) Is this supposed to stand for "Multi-bit ECC" (from "N.2.5 Memory >>> Error Section" in UEFI 2.6)? Should we have a macro for that? > >> Yes, it is. What do you mean a macro? > > A #define for the integer value 3. > >> For all the errors that happen in the guest OS, in order to simulate >> easy, I abstract all the error section to memory section, even though >> the error section is processor or other section. > Why is that a valid thing to do? (I'm not doubting it is valid, I'm just > asking.) Will that not confuse the ACPI subsystem of the guest OS? > >> I do not know whether do you have some suggestion for that. > Well I would have thought (without any expertise on the subject) that > hardware errors from the host side should be mapped to the guest more or > less "type correct". IOW, it looks strange that, say, a CPU error is > reported as a memory error. But this is just an uneducated guess. > >>>>> + mem_err->validation_bits |= CPER_MEM_VALID_CARD | CPER_MEM_VALID_MODULE | >>>>> + CPER_MEM_VALID_BANK | CPER_MEM_VALID_ROW | >>>>> + CPER_MEM_VALID_COLUMN | CPER_MEM_VALID_BIT_POSITION; >>>>> + mem_err->card = 1; >>>>> + mem_err->module = 2; >>>>> + mem_err->bank = 3; >>>>> + mem_err->row = 1; >>>>> + mem_err->column = 2; >>>>> + mem_err->bit_pos = 5; >>> >>> (60) I have no idea where these values come from. > >> For all the errors that happen in the guest OS, in order to simulate >> easy, I abstract all the error section to memory section, and hard >> code the memory section error value as above. > Sure, but why is that safe? Will the guest OS not want to do something > about these error details? If we are feeding the guest OS invalid error > details, will that not lead to confusion? > >>> (64) What does "reqr" stand for? >> It stand for the request size. > Can you please call it "req_size" or something similar? The English > expression > > request size > > contains only one "r" letter, so it's hard to understand where the > second "r" in "reqr" comes from. > > Thanks, > Laszlo > > . >
