Re: [PATCH v1 04/17] xen/riscv: introduce device-agnostic MMIO emulation dispatch

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 8/10/26 4:49 PM, Baptiste Le Duc wrote:
>> diff --git a/xen/arch/riscv/include/asm/mmio.h b/xen/arch/riscv/include/asm/mmio.h
>> new file mode 100644
>> index 0000000000..18df1133e6
>> --- /dev/null
>> +++ b/xen/arch/riscv/include/asm/mmio.h
>> @@ -0,0 +1,63 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> 
> According to coding style, it should be GPL-2.0-only.

Could you please point me to the line in the coding style document where 
this is mentioned?

If you are referring to:
   New files should start with a single-line SPDX comment to express the
   license, e.g.:

   /* SPDX-License-Identifier: GPL-2.0-only */

   See LICENSES/ for a list of licenses and SPDX tags currently used.

Then my understanding is that /* SPDX-License-Identifier: GPL-2.0-only 
*/ is used only as an example, and I can choose any license from 
LICENSES/. There, it is mentioned:
   Valid-License-Identifier: LGPL-2.0-only
   Valid-License-Identifier: LGPL-2.0-or-later

I am pretty sure that I am free to choose any license that does not 
conflict with the other licenses used in the project.

>> +#ifndef RISCV_MMIO_H
>> +#define RISCV_MMIO_H
>> +
>> +#include <xen/lib.h>
>> +#include <xen/rwlock.h>
>> +
>> +#define MAX_IO_HANDLER  16
>> +
>> +typedef struct {
>> +    paddr_t gpa;
>> +    unsigned int len;  /* access width in bytes (1, 2, 4, 8) */
>> +    bool is_write;
>> +    register_t data;   /* store: value to write; load: value read (set by handler) */
> 
> Nit: line too long (85)

I will apply that. Actually I've already fixed that by putting the 
comment above:
   /* store: value to write; load: value read (set by handler) */
   register_t data;


>> diff --git a/xen/arch/riscv/mmio.c b/xen/arch/riscv/mmio.c
>> new file mode 100644
>> index 0000000000..7d56bc8b27
>> --- /dev/null
>> +++ b/xen/arch/riscv/mmio.c
>> @@ -0,0 +1,145 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> Should be GPL-2.0-only.

Regarding license I've wrote a comment above so lets continue discussion 
there.

>> +/*
>> + * Copyright (C) Vates
>> + */
> Why have you included a copyright notice here, but not in the other
> files? 

So I just decided to do that for new files as I am not using corporate 
e-mail.

I don’t know if you can keep it,

Good point, I have to ask then someone from our legal department...

  but I just wanted to point out
> that there are other files where this type of copyright notice includes
> the year.
> 

Before, I used to include the year, but someone pointed out (or perhaps 
I misunderstood) that there isn’t much point in including it and that it 
is enough to have just (c) <company name>.

Thanks.

~ Oleksii