Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

On 2026-08-10 17:04:43+02:00, Oleksii Kurochko wrote:
> On 8/10/26 3:32 PM, Baptiste Le Duc wrote:
> 
> >> It was decided to add support for IMSIC from the start instead of having APLIC
> > 
> > Add a #include <xen/percpu.h> here instead of in aia.h.
> 
> Sorry, but I’m a little confused here. <asm/aia.h> doesn’t include 
> <xen/percpu.h>.
> 
Yes it is, in xen.git/xen/arch/riscv/asm/aia.h, you added, in this patch
    #include <xen/percpu.h>
Therefore, I think it could be included directly in the aia.c file as it
is the only place where it is used.
> > Could we call this function with a different cpu arg than the current
> > one running? If yes, we would read hgeie of not the cpu we wanted.
> 
> Considering that it touches the CSR_HGIEI register, it can only be 
> called on the currently running CPU.
> 
> That’s why I suggested in one of my replies to Jan B. that I would drop 
> the argument altogether for this function.
> 
> >> +{
> > 
> > 
> > Why `0` rather than smp_processor_id()? As described above vgein_init() reads CSR_HGEIE
> > of the current hart but stores the result into per_cpu(vgein, cpu), so the two
> > must agree.
> 
> aia_init() is executed on boot cpu only so it uses 0 as Xen boot cpu is 
> always 0. But it won't be an issue anymore as I mentioned above an 
> argument of vgein_init() will be dropped anyway so it will be guaranteed 
> that a correct CPU is used.
> 
> > What happens if v->processor change between vgein_assign() and
> > vgein_release? Because it seems in such case the release will hit a
> > different pCPU's bitmap: the original bit will leak and an unrelated
> > CPU's bit will be cleared under another vCPU's feet.
> 
> So, if v->processor changes between the calls to vgein_assign() and 
> vgein_release(), it means that migration has happened. If migration has 
> happened, then it is the responsibility of the migration code to 
> properly assign the new vgein and release the previous one.
> 
> All other cases where vgein_release() is called are when the vCPU is 
> dying, so everything is okay there as migration cannot happen.
> 
> > Potential index error, because above you did:
> > 
> >      vgein->owners = xvzalloc_array(struct vcpu*, vgein->geilen)
> > 
> > so valid index are 0...(vgein->geilen-1). Adopt either
> > one of those two options:
> >      1. vgein->owners[vgein_id-1] = v
> >      2. vgein->owners = xvzalloc_array(struct vcpu *, vgein->geilen+1) in
> >         vgein_init()
> > 
> > I think `2` could be better to have vgein->owners replicated hgeie CSR but
> > it would left the first entry read-only.
> 
> I've found that too during prepare a reply to Jan B. so fixed it already 
> in v2. I've decided to go with what you suggested in 2.
> 
> > VGEIN_DEBUG is not defined anywhere in the patch, please use
> > gdprintk(XENLOG_DEBUG, ...) directly, or drop this branch.
> 
> It is intentionally not defined. If a user needs additional VGEIN debug 
> information, they should define it themselves, as it can produce a 
> pretty large amount of logs due to, for example, the migration process, 
> where vgein_assign() and vgein_release() are used quite actively.
> 
Oh I didn't know it was a common practice, thanks for this explanation.
> > asm/aia.h needs neither <xen/percpu.h> nor <xen/spinlock.h> as struct
> > vgein_ctrl and the per-CPU variable both live in aia.c. Please drop them
> > and add <xen/percpu.h> in aia.c
> 
> Yes, it is redundant code that I missed removing. I’ve already noticed 
> it and removed it in v2.
It's what I wanted to mean in the comment above about <xen/percpu.h>
> Thanks.
Happy to help :)
> 
> ~ Oleksii