On Sat, 12 Oct 2019 11:04:03 +0800
Tao Xu <tao3...@intel.com> wrote:
> On 10/11/2019 10:08 PM, Igor Mammedov wrote:
> > On Thu, 10 Oct 2019 14:53:56 +0800
> > Tao Xu <tao3...@intel.com> wrote:
> >
> >> On 10/3/2019 10:41 PM, Igor Mammedov wrote:
> >>> On Fri, 20 Sep 2019 15:43:47 +0800
> >>> Tao Xu <tao3...@intel.com> wrote:
> >>>
> >>>> From: Liu Jingqi <jingqi....@intel.com>
> >>>>
> >>>> This structure describes the memory access latency and bandwidth
> >>>> information from various memory access initiator proximity domains.
> >>>> The latency and bandwidth numbers represented in this structure
> >>>> correspond to rated latency and bandwidth for the platform.
> >>>> The software could use this information as hint for optimization.
> >>>>
> >>>> Signed-off-by: Liu Jingqi <jingqi....@intel.com>
> >>>> Signed-off-by: Tao Xu <tao3...@intel.com>
> >>>> ---
> >>>>
> >>>> Changes in v12:
> >>>> - Fix a bug that if HMAT is enabled and without hmat-lb setting,
> >>>> QEMU will crash. (reported by Danmei Wei)
> >>>>
> >>>> Changes in v11:
> >>>> - Calculate base in build_hmat_lb().
> >>>> ---
> >>>> hw/acpi/hmat.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>> hw/acpi/hmat.h | 2 +
> >>>> 2 files changed, 127 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
> >>>> index 1368fce7ee..e7be849581 100644
> >>>> --- a/hw/acpi/hmat.c
> >>>> +++ b/hw/acpi/hmat.c
> >>>> @@ -27,6 +27,7 @@
> >>>> #include "qemu/osdep.h"
> >>>> #include "sysemu/numa.h"
> >>>> #include "hw/acpi/hmat.h"
> >>>> +#include "qemu/error-report.h"
> >>>>
> >>>> /*
> >>>> * ACPI 6.3:
> >>>> @@ -67,11 +68,105 @@ static void build_hmat_mpda(GArray *table_data,
> >>>> uint16_t flags, int initiator,
> >>>> build_append_int_noprefix(table_data, 0, 8);
> >>>> }
> >>>>
> >>>> +static bool entry_overflow(uint64_t *lb_data, uint64_t base, int len)
> >>>> +{
> >>>> + int i;
> >>>> +
> >>>> + for (i = 0; i < len; i++) {
> >>>> + if (lb_data[i] / base >= UINT16_MAX) {
> >>>> + return true;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + return false;
> >>>> +}
> >>> I suggest to do this check at CLI parsing time
> >>>
> >>>> +/*
> >>>> + * ACPI 6.3: 5.2.27.4 System Locality Latency and Bandwidth Information
> >>>> + * Structure: Table 5-146
> >>>> + */
> >>>> +static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
> >>>> + uint32_t num_initiator, uint32_t num_target,
> >>>> + uint32_t *initiator_list, int type)
> >>>> +{
> >>>> + uint8_t mask = 0x0f;
> >>>> + uint32_t s = num_initiator;
> >>>> + uint32_t t = num_target;
> >>> drop this locals and use arguments directly
> >>>
> >>>> + uint64_t base = 1;
> >>>> + uint64_t *lb_data;
> >>>> + int i, unit;
> >>>> +
> >>>> + /* Type */
> >>>> + build_append_int_noprefix(table_data, 1, 2);
> >>>> + /* Reserved */
> >>>> + build_append_int_noprefix(table_data, 0, 2);
> >>>> + /* Length */
> >>>> + build_append_int_noprefix(table_data, 32 + 4 * s + 4 * t + 2 * s *
> >>>> t, 4);
> >>> ^^^^
> >>> to me above looks like /dev/random output, absolutely unreadable.
> >>> Suggest to use local var (like: lb_length) for expression with comments
> >>> beside magic numbers.
> >>>
> >>>> + /* Flags: Bits [3:0] Memory Hierarchy, Bits[7:4] Reserved */
> >>>> + build_append_int_noprefix(table_data, hmat_lb->hierarchy & mask,
> >>>> 1);
> >>>
> >>> why do you need to use mask here?
> >>>
> >> Because Bits[7:4] Reserved, so I use mask to keep it reserved.
> >
> > these bits are not user provided and set to 0, if they get set it's
> > programming error and instead of masking problem out QEMU should abort,
> > I suggest replace masking with assert(!foo>>x).
> >
> >>
> >>>> + /* Data Type */
> >>>> + build_append_int_noprefix(table_data, hmat_lb->data_type, 1);
> >>>
> >>> Isn't hmat_lb->data_type and passed argument 'type' the same?
> >>>
> >> Yes, I will drop 'type'.
> >>>
> >>>> + /* Reserved */
> >>>> + build_append_int_noprefix(table_data, 0, 2);
> >>>> + /* Number of Initiator Proximity Domains (s) */
> >>>> + build_append_int_noprefix(table_data, s, 4);
> >>>> + /* Number of Target Proximity Domains (t) */
> >>>> + build_append_int_noprefix(table_data, t, 4);
> >>>> + /* Reserved */
> >>>> + build_append_int_noprefix(table_data, 0, 4);
> >>>> +
> >>>> + if (HMAT_IS_LATENCY(type)) {
> >>>> + unit = 1000;
> >>>> + lb_data = hmat_lb->latency;
> >>>> + } else {
> >>>> + unit = 1024;
> >>>> + lb_data = hmat_lb->bandwidth;
> >>>> + }
> >>>> +
> >>>> + while (entry_overflow(lb_data, base, s * t)) {
> >>>> + for (i = 0; i < s * t; i++) {
> >>>> + if (!QEMU_IS_ALIGNED(lb_data[i], unit * base)) {
> >>>> + error_report("Invalid latency/bandwidth input, all "
> >>>> + "latencies/bandwidths should be specified in the same
> >>>> units.");
> >>>> + exit(1);
> >>>> + }
> >>>> + }
> >>>> + base *= unit;
> >>>> + }
> >>> Can you clarify what you are trying to check here?
> >>>
> >> This part I use entry_overflow() to check if uint16 can store entry. If
> >> can't store and the entries matrix can be divisible by unit * base, then
> >> base will be unit * base.
> >>
> >> For example, if lb_data[i] are 1048576(1TB/s) and 1024(1GB/s), unit is
> >> 1024, so 1048576 is bigger than UINT16_MAX, and can be divisible by 1024
> >> * 1, so base is 1024 and entries are 1024 and 1 (see entry =
> >> hmat_lb->latency[i] / base;). The benefit is even user input different
> >> unit(TB/s vs GB/s), we can still store the data as far as possible.
> >
> > Is it possible instead of doing multiple iterations over lb_data
> > until it finds valid base, just go over lb_data once to find MIN/MAX
> > and then calculate base using it. Error out with max/min offending
> > values if it's not possible to compress the range into uint16_t?
> >
>
> Although we tell user input same unit data, such as use 1GB/s 3GB/s. If
> user input data such as 1048575, 1048576(1TB/s) and 1024(1GB/s), then we
> will get 1024 * (1023 1024 1). I am wondering if it is appropriate
> because we lose a float number(0.999020). But in our codes, it will
> raise error.
I do not understand what you are trying to say here, could you rephrase
it, so the problem would be more clear, please?