On Nov 2 11:50, Peter Maydell wrote: > On Thu, 30 Oct 2025 at 07:30, Klaus Jensen <[email protected]> wrote: > > > > From: Alan Adamson <[email protected]> > > > > Add support for the namespace atomic paramters: NAWUN and NAWUN. Namespace > > Atomic Compare and Write Unit (NACWU) is not currently supported. > > > > Writes that adhere to the NACWU and NAWUPF parameters are guaranteed to be > > atomic. > > > > New NVMe QEMU Paramters (See NVMe Specification for details): > > atomic.nawun=UINT16 (default: 0) > > atomic.nawupf=UINT16 (default: 0) > > atomic.nsfeat (default off) - Set Namespace Supported Atomic > > Boundary & > > Power (NSABP) bit in Namespace Features (NSFEAT) in the > > Identify > > Namespace Data Structure > > > > See the NVMe Specification for more information. > > > > Signed-off-by: Alan Adamson <[email protected]> > > Reviewed-by: Jesper Wendel Devantier <[email protected]> > > Signed-off-by: Klaus Jensen <[email protected]> > > > > > + /* Set atomic write parameters */ > > + if (ns->params.atomic_nsfeat) { > > + id_ns->nsfeat |= NVME_ID_NS_NSFEAT_NSABPNS; > > + id_ns->nawun = cpu_to_le16(ns->params.atomic_nawun); > > + if (!id->awupf || (id_ns->nawun && (id_ns->nawun < id->awun))) { > > + error_report("Invalid NAWUN: %x AWUN=%x", id_ns->nawun, > > id->awun); > > + } > > This error check is about NAWUN, but the condition looks at id->awpuf. > Is that intentional ? (Coverity wonders if this is a copy-paste error: > CID 1642811.) >
The check is as intended, but I can understand why Coverity thinks it
may be off. I'll rework it.
> We should return early if we've detected an error case in the properties.
> By default we'll fall on through. Similarly below.
>
> This is a realize method, so error handling should be by
> setting the 'error' argument, not by error_report().
>
True, I'll fix that up.
> > + id_ns->nawupf = cpu_to_le16(ns->params.atomic_nawupf);
> > + if (!id->awupf || (id_ns->nawupf && (id_ns->nawupf < id->awupf))) {
> > + error_report("Invalid NAWUPF: %x AWUPF=%x",
> > + id_ns->nawupf, id->awupf);
> > + }
> > + if (id_ns->nawupf > id_ns->nawun) {
> > + error_report("Invalid: NAWUN=%x NAWUPF=%x",
> > + id_ns->nawun, id_ns->nawupf);
> > + }
>
> Personally I find this stack of checks a bit confusing -- we
> are presumably catching various different invalid combinations
> of the properties, but the error messages we produce are rather
> unspecific. If it's the case that (for instance) the NAWUPF
> cannot be larger than the NAWUN, we could tell the user that
> specifically rather than just saying "Invalid" and making them
> go look up what the requirements are in the spec or the code.
>
I'll fix up the parameter validation.
Thanks Peter!
signature.asc
Description: PGP signature
