| Message ID | 1575479147-6641-5-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | q35: CPU hotplug with secure boot, part 1+2 | expand |
On 12/04/19 18:05, Igor Mammedov wrote: > Correct returned value description in case 'Command field' == 0x0, > it's in not PXM but CPU selector value with pending event (1) s/in not/not/ > > In addition describe 0 blanket value in case of not supported > 'Command field' value. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > docs/specs/acpi_cpu_hotplug.txt | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > index 4e65286..19c508f 100644 > --- a/docs/specs/acpi_cpu_hotplug.txt > +++ b/docs/specs/acpi_cpu_hotplug.txt > @@ -56,9 +56,8 @@ read access: > 3-7: reserved and should be ignored by OSPM > [0x5-0x7] reserved > [0x8] Command data: (DWORD access) > - in case of error or unsupported command reads is 0xFFFFFFFF > - current 'Command field' value: > - 0: returns PXM value corresponding to device > + contains 0 unless last stored in 'Command field' value is one of: > + 0: contains 'CPU selector' value of a CPU with pending event[s] (2) I think we can improve the word order: last stored in 'Command field' value -> value last stored in 'Command field' > > write access: > offset: > @@ -81,9 +80,9 @@ write access: > value: > 0: selects a CPU device with inserting/removing events and > following reads from 'Command data' register return > - selected CPU (CPU selector value). If no CPU with events > - found, the current CPU selector doesn't change and > - corresponding insert/remove event flags are not set. > + selected CPU ('CPU selector' value). > + If no CPU with events found, the current 'CPU selector' doesn't > + change and corresponding insert/remove event flags are not set. (3) AFAICT this is only a -- useful! -- re-wrapping. But, since we are modifying this section anyway, can we replace "flags are not set" with "flags are left unchanged" or "flags are not modified"? "set" is ambiguous with bit fields: it can mean "rewritten", and it can mean "set to 1". > 1: following writes to 'Command data' register set OST event > register in QEMU > 2: following writes to 'Command data' register set OST status > Anyway, these are all superficial comments. Pick up whatever you agree with. Either way: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo
On Thu, 5 Dec 2019 13:17:22 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 12/04/19 18:05, Igor Mammedov wrote: > > Correct returned value description in case 'Command field' == 0x0, > > it's in not PXM but CPU selector value with pending event > > (1) s/in not/not/ > > > > > In addition describe 0 blanket value in case of not supported > > 'Command field' value. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > docs/specs/acpi_cpu_hotplug.txt | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt > > index 4e65286..19c508f 100644 > > --- a/docs/specs/acpi_cpu_hotplug.txt > > +++ b/docs/specs/acpi_cpu_hotplug.txt > > @@ -56,9 +56,8 @@ read access: > > 3-7: reserved and should be ignored by OSPM > > [0x5-0x7] reserved > > [0x8] Command data: (DWORD access) > > - in case of error or unsupported command reads is 0xFFFFFFFF > > - current 'Command field' value: > > - 0: returns PXM value corresponding to device > > + contains 0 unless last stored in 'Command field' value is one of: > > + 0: contains 'CPU selector' value of a CPU with pending event[s] > > (2) I think we can improve the word order: > > last stored in 'Command field' value > -> > value last stored in 'Command field' > > > > > write access: > > offset: > > @@ -81,9 +80,9 @@ write access: > > value: > > 0: selects a CPU device with inserting/removing events and > > following reads from 'Command data' register return > > - selected CPU (CPU selector value). If no CPU with events > > - found, the current CPU selector doesn't change and > > - corresponding insert/remove event flags are not set. > > + selected CPU ('CPU selector' value). > > + If no CPU with events found, the current 'CPU selector' doesn't > > + change and corresponding insert/remove event flags are not set. > > (3) AFAICT this is only a -- useful! -- re-wrapping. Not sure what you are trying to say here ... > But, since we are > modifying this section anyway, can we replace "flags are not set" with > "flags are left unchanged" or "flags are not modified"? sure > "set" is ambiguous with bit fields: it can mean "rewritten", and it can > mean "set to 1". > > > 1: following writes to 'Command data' register set OST event > > register in QEMU > > 2: following writes to 'Command data' register set OST status > > > > Anyway, these are all superficial comments. Pick up whatever you agree > with. Either way: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Thanks! > Laszlo
On 12/06/19 12:09, Igor Mammedov wrote: > On Thu, 5 Dec 2019 13:17:22 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 12/04/19 18:05, Igor Mammedov wrote: >>> Correct returned value description in case 'Command field' == 0x0, >>> it's in not PXM but CPU selector value with pending event >> >> (1) s/in not/not/ >> >>> >>> In addition describe 0 blanket value in case of not supported >>> 'Command field' value. >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>> --- >>> docs/specs/acpi_cpu_hotplug.txt | 11 +++++------ >>> 1 file changed, 5 insertions(+), 6 deletions(-) >>> >>> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt >>> index 4e65286..19c508f 100644 >>> --- a/docs/specs/acpi_cpu_hotplug.txt >>> +++ b/docs/specs/acpi_cpu_hotplug.txt >>> @@ -56,9 +56,8 @@ read access: >>> 3-7: reserved and should be ignored by OSPM >>> [0x5-0x7] reserved >>> [0x8] Command data: (DWORD access) >>> - in case of error or unsupported command reads is 0xFFFFFFFF >>> - current 'Command field' value: >>> - 0: returns PXM value corresponding to device >>> + contains 0 unless last stored in 'Command field' value is one of: >>> + 0: contains 'CPU selector' value of a CPU with pending event[s] >> >> (2) I think we can improve the word order: >> >> last stored in 'Command field' value >> -> >> value last stored in 'Command field' >> >>> >>> write access: >>> offset: >>> @@ -81,9 +80,9 @@ write access: >>> value: >>> 0: selects a CPU device with inserting/removing events and >>> following reads from 'Command data' register return >>> - selected CPU (CPU selector value). If no CPU with events >>> - found, the current CPU selector doesn't change and >>> - corresponding insert/remove event flags are not set. >>> + selected CPU ('CPU selector' value). >>> + If no CPU with events found, the current 'CPU selector' doesn't >>> + change and corresponding insert/remove event flags are not set. >> >> (3) AFAICT this is only a -- useful! -- re-wrapping. > Not sure what you are trying to say here ... I meant that you re-wrapped the last sentence, by breaking it off to a separate line. And that I found it useful. Thanks laszlo > >> But, since we are >> modifying this section anyway, can we replace "flags are not set" with >> "flags are left unchanged" or "flags are not modified"? > sure > > >> "set" is ambiguous with bit fields: it can mean "rewritten", and it can >> mean "set to 1". >> >>> 1: following writes to 'Command data' register set OST event >>> register in QEMU >>> 2: following writes to 'Command data' register set OST status >>> >> >> Anyway, these are all superficial comments. Pick up whatever you agree >> with. Either way: >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> >> Thanks! >> Laszlo >
diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt index 4e65286..19c508f 100644 --- a/docs/specs/acpi_cpu_hotplug.txt +++ b/docs/specs/acpi_cpu_hotplug.txt @@ -56,9 +56,8 @@ read access: 3-7: reserved and should be ignored by OSPM [0x5-0x7] reserved [0x8] Command data: (DWORD access) - in case of error or unsupported command reads is 0xFFFFFFFF - current 'Command field' value: - 0: returns PXM value corresponding to device + contains 0 unless last stored in 'Command field' value is one of: + 0: contains 'CPU selector' value of a CPU with pending event[s] write access: offset: @@ -81,9 +80,9 @@ write access: value: 0: selects a CPU device with inserting/removing events and following reads from 'Command data' register return - selected CPU (CPU selector value). If no CPU with events - found, the current CPU selector doesn't change and - corresponding insert/remove event flags are not set. + selected CPU ('CPU selector' value). + If no CPU with events found, the current 'CPU selector' doesn't + change and corresponding insert/remove event flags are not set. 1: following writes to 'Command data' register set OST event register in QEMU 2: following writes to 'Command data' register set OST status
Correct returned value description in case 'Command field' == 0x0, it's in not PXM but CPU selector value with pending event In addition describe 0 blanket value in case of not supported 'Command field' value. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- docs/specs/acpi_cpu_hotplug.txt | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)