Re: [PATCH v2 5/9] lib: sbi_hart: add WID protection mechanism

[Yu-Chien Peter Lin <peter.lin@sifive.com>]

Hi Radim,

On Fri, Oct 16, 2026 at 05:24:07PM +0000, Radim Krcmar wrote:
> 2026-08-17T17:04:59+08:00, Yu-Chien Peter Lin <peter.lin@sifive.com>:
> > Register a WID-based hart protection mechanism that
> > configures mlwid and mwiddeleg CSRs on domain entry.
> > This enables the protection framework reconfigure WID
> > isolation whenever domains are switched.
> >
> > Signed-off-by: Yu-Chien Peter Lin <peter.lin@sifive.com>
> > ---
> > Changes v1 -> v2:
> > - Validate WID against pmlwidlist before writing mlwid CSR (Oza)
> > ---
> > diff --git a/lib/sbi/sbi_hart_worlds.c b/lib/sbi/sbi_hart_worlds.c
> > +++ b/lib/sbi/sbi_hart_worlds.c
> > @@ -0,0 +1,72 @@
> > +/*
> > + * SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright (c) 2026 SiFive Inc.
> > + */
> > +
> > +#include <sbi/riscv_encoding.h>
> > +#include <sbi/sbi_console.h>
> > +#include <sbi/sbi_domain.h>
> > +#include <sbi/sbi_error.h>
> > +#include <sbi/sbi_hart.h>
> > +#include <sbi/sbi_hart_protection.h>
> > +#include <sbi/sbi_hart_worlds.h>
> > +#include <sbi/sbi_scratch.h>
> > +
> > +static int sbi_hart_worlds_configure(struct sbi_scratch *scratch,
> > +				     struct sbi_domain *dom)
> > +{
> > +	struct sbi_hart_features *hf = sbi_hart_features_ptr(scratch);
> > +	bool wid_found = true;
> > +	u32 wid_val = 0;
> > +
> > +	if (!sbi_hart_has_extension(scratch, SBI_HART_EXT_SMLWID))
> > +		return 0;
> > +	if (dom->has_wid) {
> > +		wid_val = dom->wid;
> 
> I would prefer to fail domain creation if we have mlwid and the domain
> doesn't specify its wid.
> 
> The rest are security issues waiting to happen.

Agreed. I’ll make WID mandatory whenever MLWID is present, rather than
falling back to other sources.

> 
> > +	} else if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SMWID)) {
> > +		wid_val = csr_read(CSR_MWID) & ~MWID_LOCK;
> > +	} else if (hf->has_pmwid) {
> > +		wid_val = hf->pmwid;
> > +	} else {
> > +		/*
> > +		 * Smlwid present but no WID source. Without writing
> > +		 * mlwid, the hardware reset value is used which may
> > +		 * cause a software-check exception on lower-privilege
> > +		 * mode entry.
> > +		 */
> > +		wid_found = false;
> > +	}
> 
> mlwid CSR is a WID source by itself -- the domain will use the current
> value...  Failing initialization seems better than reusing whatever the
> last domain left there.

Agreed.

> 
> > +
> > +	if (wid_found) {
> > +		if (hf->has_pmlwidlist &&
> > +		    !(hf->pmlwidlist & BIT_ULL(wid_val))) {
> > +			sbi_printf("%s: domain wid %u not in pmlwidlist\n",
> > +				   __func__, wid_val);
> > +			return SBI_EINVAL;
> > +		}
> > +		csr_write(CSR_MLWID, wid_val);
> > +	}
> > +
> > +	if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SMWIDDELEG))
> > +		csr_write(CSR_MWIDDELEG, dom->widdeleg);
> 
> I would be a happier if we added a little bit of widdeleg verification,
> so DT mistakes wouldn't shoot us in the head.

Sure.

Regards,
Peter Lin

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi