On Fri, Sep 06, 2024 at 10:34:44AM -0300, Jason Gunthorpe wrote:

On Fri, Sep 06, 2024 at 11:07:47AM +0000, Mostafa Saleh wrote:

quoted

However, I believe the UAPI can be more clear and solid in terms of
what is supported (maybe a typical struct with the CD, and some
extra configs?) I will give it a think.

I don't think breaking up the STE into fields in another struct is
going to be a big improvement, it adds more code and corner cases to
break up and reassemble it.

#define STRTAB_STE_0_NESTING_ALLOWED                                         \
	cpu_to_le64(STRTAB_STE_0_V | STRTAB_STE_0_CFG | STRTAB_STE_0_S1FMT | \
		    STRTAB_STE_0_S1CTXPTR_MASK | STRTAB_STE_0_S1CDMAX)
#define STRTAB_STE_1_NESTING_ALLOWED                            \
	cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |   \
		    STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |   \
		    STRTAB_STE_1_S1STALLD | STRTAB_STE_1_EATS)

It is 11 fields that would need to be recoded, that's alot.. Even if
you say the 3 cache ones are not needed it is still alot.

I was thinking of providing a higher level semantics
(no need for caching, valid...), something like:

struct smmu_user_table {
	u64 cd_table;
	u32 smmu_cd_cfg;  /* linear or 2lvl,.... */
	u32 smmu_trans_cfg; /* Translate, bypass, abort */
	u32 dev_feat; /*ATS, STALL, …*/
};

I feel that is a bit more clear for user space? Instead of
partially setting the STE, and it should be easier to extend than
masking the STE.

I’am not opposed to the vSTE, I just feel it's loosely defined,
that's why I was asking for the docs.

quoted

quoted

Reporting a static kernel capability through GET_INFO output is
easier/saner than providing some kind of policy flags in the GET_INFO
input to specify how the sanitization should work.

I don’t think it’s “policy”, it’s just giving userspace the minimum
knowledge it needs to create the vSMMU, but again no really strong
opinion about that.

There is no single "minimum knowledge" though, it depends on what the
VMM is able to support. IMHO once you go over to the "VMM has to
ignore bits it doesn't understand" you may as well just show
everything. Then the kernel side can't be wrong.

If the kernel side can be wrong, then you are back to handshaking
policy because the kernel can't assume that all existing VMMs wil not
rely on the kernel to do the masking.

I agree it’s tricky, again no strong opinion on that, although I doubt
that a VMM would care about all the SMMU features.

quoted

quoted

quoted

But this is a UAPI. How can userspace implement that if it has no
documentation, and how can it be maintained if there is no clear
interface with userspace with what is expected/returned...

I'm not sure what you are looking for here? I don't think an entire
tutorial on how to build a paravirtualized vSMMU is appropriate to
put in comments?

Sorry, I don’t think I was clear, I meant actual documentation for
the UAPI, as in RST files for example. If I want to support that
in kvmtool how can I implement it? 

Well, you need thousands of lines of code in kvtool to build a vIOMMU :)

Nicolin is looking at writing something, lets see.

I think for here we should focus on the comments being succinct but
sufficient to understand what the uAPI does itself.

Actually I think the opposite, I think UAPI docs is more important
here, especially for the vSTE, that's how we can compare the code to
what is expected from user-space.

quoted

quoted

I would *really* like everyone to sit down and figure out how to
manage virtual device lifecycle in a single language!

Yes, just like the guest_memfd work. There has been also
some work to unify some of the guest HVC bits:
https://lore.kernel.org/all/20240830130150.8568-1-will@kernel.org/ (local)

I think Dan Williams is being ringleader for the PCI side effort on CC

Thanks, I will try to spend some time on the secure VFIO work.

Thanks,
Mostafa

Jason