Re: [RFC PATCH] virtio-iommu: Add PAGE_SIZE_MASK property

[Auger Eric <eric.auger@redhat.com>]

Hi Jean,

On 3/23/20 2:38 PM, Jean-Philippe Brucker wrote:
> Add a PROBE property to declare the mapping granularity per endpoint.
> The virtio-iommu device already declares a granule in its config space,
> but when endpoints are behind different physical IOMMUs, they may have
> different mapping granules. This new property allows to override the
> global page_size_mask for each endpoint.
> 
> In the future it may be useful to describe more than one page_size_mask
> for each endpoint, and allow them to negotiate it during ATTACH. For
> example two masks could allow the driver to choose between 4k and 64k
> granule, along with their respective block mapping sizes. This could be
> added by replacing \field{reserved} with an array length, for example.
Sorry I don't get the use case where several page size bitmaps should be
exposed.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> I had this change planned for a page-table sharing extension, but it
> seems to be useful for the VFIO support in QEMU as well [1].
> 
> [1] https://lore.kernel.org/qemu-devel/3da60c1b-6897-7ab1-3a67-bec44fa00a54@redhat.com/
> ---
> ---
>  virtio-iommu.tex | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/virtio-iommu.tex b/virtio-iommu.tex
> index 08b358a..d7be13d 100644
> --- a/virtio-iommu.tex
> +++ b/virtio-iommu.tex
> @@ -664,7 +664,8 @@ \subsubsection{PROBE request}\label{sec:Device Types / IOMMU Device / Device ope
>  \subsubsection{PROBE properties}\label{sec:Device Types / IOMMU Device / Device operations / PROBE properties}
>  
>  \begin{lstlisting}
> -#define VIRTIO_IOMMU_PROBE_T_RESV_MEM   1
> +#define VIRTIO_IOMMU_PROBE_T_RESV_MEM         1
> +#define VIRTIO_IOMMU_PROBE_T_PAGE_SIZE_MASK   2
>  \end{lstlisting}
>  
>  \paragraph{Property RESV_MEM}\label{sec:Device Types / IOMMU Device / Device operations / PROBE properties / RESVMEM}
> @@ -727,6 +728,36 @@ \subsubsection{PROBE properties}\label{sec:Device Types / IOMMU Device / Device
>  The device SHOULD NOT allow accesses from the endpoint to RESV_MEM regions
>  to affect any other component than the endpoint and the driver.
>  
> +\paragraph{Property PAGE_SIZE_MASK}\label{sec:Device Types / IOMMU Device / Device operations / PROBE properties / PAGESIZEMASK}
"Property PAGESIZEMASK" to be homegeneous with "Property RESV_MEM"?
> +
> +The PAGE_SIZE_MASK property overrides the global \field{page_size_mask}
> +configuration for an endpoint.
> +
> +\begin{lstlisting}
> +struct virtio_iommu_probe_page_size_mask {
> +  struct virtio_iommu_probe_property head;
> +  u8    reserved[4];
> +  le64  page_size_mask;
> +};
> +\end{lstlisting}

Maybe it would be useful to put in the spec the context of use of the
new property (basically what you wrote in the commit msg). Also
re-inforce that if no ep page size mask is returned the default one
applies. Chapters describing page_size_mask global field may be sligtly
augmented to mention the new prop...
> +
> +The \field{page_size_mask} field behaves in the same way as the global
> +\field{page_size_mask} field. The least significant bit describes the
> +mapping granularity, and additional bits are hints (see \ref{sec:Device
> +Types / IOMMU Device / Device operations})
> +
> +\drivernormative{\subparagraph}{Property PAGE_SIZE_MASK}{Device Types / IOMMU Device / Device operations / PROBE properties / PAGESIZEMASK}
> +
> +The driver MUST ignore \field{reserved}.
> +
> +\devicenormative{\subparagraph}{Property PAGE_SIZE_MASK}{Device Types / IOMMU Device / Device operations / PROBE properties / PAGESIZEMASK}
> +
> +The device SHOULD set \field{reserved} to zero.
> +
> +The device MUST set at least one bit in \field{page_size_mask}, describing
> +the page granularity. The device MAY set more than one bit in
> +\field{page_size_mask}.
> +
>  \subsubsection{Fault reporting}\label{sev:Device Types / IOMMU Device / Device operations / Fault reporting}
>  
>  The device can report translation faults and other significant
> 

Thanks

Eric