Re: [PATCH 11/11] hw/intc/loongarch_ipi: Fix mail send and any send function

[gaosong]

On 2022/7/4 下午1:37, Richard Henderson wrote:

> On
>       7/1/22 15:04, Xiaojuan Yang wrote:
>       
> > By the document of ipi mailsend device,
> >         byte is written only when the mask bit
> >         
> >         is 0. The original code discards mask bit and overwrite the data
> >         always, this
> >         
> >         patch fixes the issue.
> >         
> >         
> >         Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> >         
> >         ---
> >         
> >           hw/intc/loongarch_ipi.c | 45
> >         ++++++++++++++++++++++-------------------
> >         
> >           1 file changed, 24 insertions(+), 21 deletions(-)
> >         
> >         
> >         diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
> >         
> >         index 553e88703d..e4b1fb5366 100644
> >         
> >         --- a/hw/intc/loongarch_ipi.c
> >         
> >         +++ b/hw/intc/loongarch_ipi.c
> >         
> >         @@ -50,35 +50,40 @@ static uint64_t loongarch_ipi_readl(void
> >         *opaque, hwaddr addr, unsigned size)
> >         
> >               return ret;
> >         
> >           }
> >         
> >           -static int get_ipi_data(target_ulong val)
> >         
> >         +static void send_ipi_data(CPULoongArchState *env, target_ulong
> >         val, target_ulong addr)
> >         
> >           {
> >         
> >               int i, mask, data;
> >         
> >           -    data = "" >> 32;
> >         
> >         -    mask = (val >> 27) & 0xf;
> >         
> >         -
> >         
> >         +    data = ""
> >         addr,
> >         
> >         +                             MEMTXATTRS_UNSPECIFIED, NULL);
> >         
> >         +    mask  = 0;
> >         
> >               for (i = 0; i < 4; i++) {
> >         
> >         -        if ((mask >> i) & 1) {
> >         
> >         -            data &= ~(0xff << (i * 8));
> >         
> >         +        /* bit 27 - 30 is mask for byte write */
> >         
> >         +        if (val & (0x1UL << (27 + i))) {
> >         
> >       
>       
>       UL suffix is never correct, since it means different things on
>       different hosts.
>       
>       Anyway, you don't any suffix here.
>       
>       
>

OK, we will remove the suffix there.

> How
>       often does mask == 0, so that all of val is written?  In which
>       case you could skip the load.
>       
>       
>     
    At most time the mask is always 0, so  we add a condition to skip
    the load.
    like this:
    +    int i, mask = 0, data = "">
    ...
    
    + 
        /*
    + 
         * bit 27-30 is mask for byte writing,
    + 
         * if the mask is 0, we should do nothing.
    + 
         */
    + 
        if ((val >> 27) & 0xf) {
    + 
            data = ""
      addr,
    + 
                                     MEMTXATTRS_UNSPECIFIED, NULL);
     ...
    }
    
    After fix these problem, should we only send these two patches? 
    
    Thanks.
    Song Gao
> r~
>       
>