Re: [PATCH v12 09/11] hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s)

[Igor Mammedov]

On Sat, 12 Oct 2019 11:04:03 +0800
Tao Xu <address@hidden> wrote:

> On 10/11/2019 10:08 PM, Igor Mammedov wrote:
> > On Thu, 10 Oct 2019 14:53:56 +0800
> > Tao Xu <address@hidden> wrote:
> >   
> >> On 10/3/2019 10:41 PM, Igor Mammedov wrote:  
> >>> On Fri, 20 Sep 2019 15:43:47 +0800
> >>> Tao Xu <address@hidden> wrote:
> >>>      
> >>>> From: Liu Jingqi <address@hidden>
> >>>>
> >>>> 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 <address@hidden>
> >>>> Signed-off-by: Tao Xu <address@hidden>
> >>>> ---
> >>>>
> >>>> 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?