Re: [PATCH v3 01/20] xen/riscv: implement sbi_remote_hfence_gvma()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.08.2025 16:45, Oleksii Kurochko wrote:
> On 8/4/25 3:52 PM, Jan Beulich wrote:
>> On 31.07.2025 17:58, Oleksii Kurochko wrote:
>>> + * Returns 0 if IPI was sent to all the targeted harts successfully
>>> + * or negative value if start_addr or size is not valid.
>> This similarly is ambiguous: The union of the success case stated and the
>> error case stated isn't obviously all possible states. The success
>> statement in particular alludes to the possibility of an IPI not actually
>> reaching its target.
> 
> The same as above this is what SBI spec. tells.
> 
> I've not checked SBI code deeply, but it seems like the code is waiting while
> IPI will be reached as looking at the code:
>       /**
>        * As this this function only handlers scalar values of hart mask, it 
> must be
>        * set to all online harts if the intention is to send IPIs to all the 
> harts.
>        * If hmask is zero, no IPIs will be sent.
>        */
>       int sbi_ipi_send_many(ulong hmask, ulong hbase, u32 event, void *data)
>       {
>                  ...
>       
>               /* Send IPIs */
>               do {
>                       retry_needed = false;
>                       sbi_hartmask_for_each_hart(i, &target_mask) {
>                               rc = sbi_ipi_send(scratch, i, event, data);
>                               if (rc == SBI_IPI_UPDATE_RETRY)
>                                       retry_needed = true;
>                               else
>                                       sbi_hartmask_clear_hart(i, 
> &target_mask);
>                       }
>               } while (retry_needed);
>       
>               /* Sync IPIs */
>               sbi_ipi_sync(scratch, event);
>       
>               return 0;
>       }
> and
>       static int sbi_ipi_sync(struct sbi_scratch *scratch, u32 event)
>       {
>               const struct sbi_ipi_event_ops *ipi_ops;
>       
>               if ((SBI_IPI_EVENT_MAX <= event) ||
>                   !ipi_ops_array[event])
>                       return SBI_EINVAL;
>               ipi_ops = ipi_ops_array[event];
>       
>               if (ipi_ops->sync)
>                       ipi_ops->sync(scratch);
>       
>               return 0;
>       }
> which calls:
>       static void tlb_sync(struct sbi_scratch *scratch)
>       {
>               atomic_t *tlb_sync =
>                               sbi_scratch_offset_ptr(scratch, tlb_sync_off);
>       
>               while (atomic_read(tlb_sync) > 0) {
>                       /*
>                        * While we are waiting for remote hart to set the sync,
>                        * consume fifo requests to avoid deadlock.
>                        */
>                       tlb_process_once(scratch);
>               }
>       
>               return;
>       }

I'll leave that comment as-is then, even if I'm not really happy with it.

>>> + * The remote fence operation applies to the entire address space if 
>>> either:
>>> + *  - start_addr and size are both 0, or
>>> + *  - size is equal to 2^XLEN-1.
>> Whose XLEN is this? The guest's? The host's? (I assume the latter, but it's
>> not unambiguous, unless there's specific terminology that I'm unaware of,
>> yet which would make this unambiguous.)
> 
> RISC-V spec quite mixes the terminology (3.1.6.2. Base ISA Control in mstatus 
> Register)
> around XLEN:
>    For RV64 harts, the SXL and UXL fields are WARL fields that control the 
> value
>    of XLEN for S-mode and U-mode, respectively. The encoding of these fields 
> is
>    the same as the MXL field of misa, shown in Table 9. The effective XLEN in
>    S-mode and U-mode are termed SXLEN and UXLEN, respectively
> 
> Basically, RISC-V privileged architecture defines different XLEN values for
> various privilege modes:
>   - MXLEN for Machine mode
>   - SXLEN for Supervisor mode.
>   - HSXLEN for Hypervisor-Supervisor mode.
>   - VSXLEN for Virtual Supervisor mode.
> 
> Considering that SBI is an API that is provided for S-mode I expect that XLEN 
> = SXLEN
> in this case, but SBI spec. is using just XLEN.

Very helpful.

Jan