Re: [PATCH v2 05/13] KVM: arm64: Detect (via ACPI) and initialize HACDBSIRQ

[Leonardo Bras]

On Mon, Jun 29, 2026 at 10:22:12AM -0700, Oliver Upton wrote:
> On Mon, Jun 29, 2026 at 12:17:53PM +0100, Leonardo Bras wrote:
> > Find via ACPI [1] the Id for HACDBSIRQ, initialize it as a per-cpu IRQ
> > and make sure any cpu able to run virtualization has it active.
> > 
> > Introduce a per-cpu structure used by the HACDBSIRQ handler to keep track
> > of entries size and the status of HACDBS. Size is used to detect end of
> > processing in case the number of entries being processed is different of
> > the supported entries size.
> > 
> > Status may look easily replaceable by checking HACDBS registers now, but
> > will make the OFF/IDLE detection easier in next patches.
> > 
> > Signed-off-by: Leonardo Bras <leo.bras@xxxxxxx>
> > 
> > [1] https://github.com/tianocore/edk2/issues/12409
> 
> Reference the ACPI specification instead please.

It's not accepted yet, unfortunately. I commented that into the cover 
letter, but forgot to add it here.

> Any link you want to
> include in a changelog should use the Link: footer, the linkage to the
> inline citation will be obvious.

Sure, will remember that in the future.

> 
> If we need to initialize the IRQ I'd really like to see device tree
> bindings for HACDBSIRQ as well. Pretty much any system us plebs can get
> our hands on is gonna be DT anyway.

Agree. I started out with ACPI because that's what the main target is, as 
dirty-logging is focused in Live Migration, which is usually more 
appreciated in the server space, which generally uses ACPI.

I spoke to some people, and I could not hear of anyone releasing a product 
based in DT that would implement this yet, so I postponed the DT 
enablement.

> 
> > +static irqreturn_t hacdbsirq_handler(int irq, void *pcpu)
> > +{
> > +	u64 cons = read_sysreg_s(SYS_HACDBSCONS_EL2);
> > +	unsigned long err = FIELD_GET(HACDBSCONS_EL2_ERR_REASON, cons);
> > +
> > +	switch (err) {
> > +	case HACDBSCONS_EL2_ERR_REASON_NOF:
> > +		this_cpu_write(hacdbs_pcp.status, HACDBS_IDLE);
> > +		break;
> > +	case HACDBSCONS_EL2_ERR_REASON_IPAHACF:
> > +		/* When size not a power of two >= 4k, exit with reserved TTLW */
> > +		int index = FIELD_GET(HACDBSCONS_EL2_INDEX, cons);
> > +
> > +		if (index >= this_cpu_read(hacdbs_pcp.size)) {
> > +			this_cpu_write(hacdbs_pcp.status, HACDBS_IDLE);
> > +			break;
> > +		}
> > +		fallthrough;
> > +	case HACDBSCONS_EL2_ERR_REASON_STRUCTF:
> > +	case HACDBSCONS_EL2_ERR_REASON_IPAF:
> > +		this_cpu_write(hacdbs_pcp.status, HACDBS_ERROR);
> > +		break;
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> I have a pretty extreme distaste for creating a state machine between
> the callsite and the IRQ handler. The callsite should poll HACDBS for
> completion. The thread has nothing better to do anyway.

Well, there is one argument it could just wait and save some energy, but I 
agree it is not relevant in server space. The main reason I did this is 
because I am planning on later doing an improved version of this that would 
clean the dirty-bit *while* running the guest, and having the IRQ is needed 
for exiting guest so we can notify userspace the cleaning is done. So I 
laid the HACDBSIRQ infra here so we don't have both polling and IRQ options 
happening. 

That idea would require us to add new API (a return value for 'cleaned'), 
and also a new flag for the clean ioctl. We also need the VMM to 
implement that, but then we get a proper cpu usage of cleaning time.

I wanted to start with a backwards compatible version, and do the above 
idea once I put my hands in hardware that implements HACDBS, so I can 
properly measure how much performance we get on above strategy.

What do you think?

Thanks!
Leo